Skip to content

Conversation

fu050409
Copy link
Contributor

  1. await new Promise(resolve => setTimeout(resolve, 100)); is confused, and this depend on networks so sometimes takes no effect. I replaced this logic with an onLoad event callback to handle this issue.
  2. Only Safari should append images, so adding append variable is redundant.

@tinchox5
Copy link
Member

Taking a flight in some minutes. I will review your PR later but please if possible check your auto lint code because there are many style changes that interfere with your proposal. Thank you again!

@fu050409
Copy link
Contributor Author

Taking a flight in some minutes. I will review your PR later but please if possible check your auto lint code because there are many style changes that interfere with your proposal. Thank you again!

Apart from what I mentioned above, the remaining content are automatically formatted by the IDE plugin (our company enforces this practice). In fact, if you don’t mind, I’d like to set up a formatting tool for snapDOM as well. I’ve encountered numerous inconsistencies in indentation and other formatting issues while reading the code, which has left me quite confused.

Wishing you a pleasant flight.

Copy link
Member

@tinchox5 tinchox5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timeout is set because of a very Safari way to resolve a kind of lazy encoding. It is not related with onload. Nevertheless I will test it because it would be nice if it is working consistently. Regarding format style I prefer to not make changes until I rewrite most of current code as is planned in the roadmap.

@fu050409
Copy link
Contributor Author

The timeout is set because of a very Safari way to resolve a kind of lazy encoding. It is not related with onload. Nevertheless I will test it because it would be nice if it is working consistently. Regarding format style I prefer to not make changes until I rewrite most of current code as is planned in the roadmap.

Thank you for your clarification! It seems I did have some misunderstandings. Many behaviors of Safari have been quite frustrating for me. The code does run normally on my device, but I can't rule out the possibility that this is a coincidental success due to environmental issues. I will do more tests on different devices later to ensure it works properly.

@tinchox5
Copy link
Member

Yeah Safari is frustrating for me too 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants