-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Dimensions2d class #54
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angusm it looks good.
I see some duplication with the yano IS class. So check that out if you haven't yet.
I'm okay with adding the user-agent stuff since it in a subdirectory /user-agent and might be good for cases where you just want pieces.
Please check this since you need it but could you later go back and add some documentation for some of the clases? Just a comment at the top of each class and some quick examples helps us read through it.
Went through with aims to consolidate and comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like x/x.ts was deleted.
@angusm Nice work. I think some of these classes are useful. One overall comment, I have is that a lot of the caching classes might come at the cost of too much abstraction making it harder for others to navigate through code without heavily investing learning the library. Part of me says, we should keep it simple and use straight native javascript read / writes combined in raf.read / raf.write - which isn't as optimized but simpler and easier to follow. However, I do see a real need to cache things like scroll, window so I see those as very useful. I think this is good to go but since it does come at a cost of abstraction, just want to make sure we continue to make efforts to add docs / examples / tests to ease the cost of onboarding for others. Thank you! |
|
Would adding helper functions to dom like there is currently for Sort of bury the caching in a black box users don't need to worry about, beyond "use these functions instead of native calls for best results"? |
Just to make sure I'm chiming in and adding my bits. This is general feedback I want us all to apply to get on the same page with our code style and approach. :)
|
Let me re-work DynamicDefaultMap. It's meant to behave as defaultdict from python. The inheritance for these map classes was such a mess because at one point ES6 Map wouldn't allow itself to be extended directly. microsoft/TypeScript#10853 That seems to have been fixed in recent browser updates :) so this can be made much less messy. Can you be specific about I agree fully with |
Remove sticky specific functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few more comments.
@uxder, i dunno what your file naming conventions are, but i'm thinking maybe |
Yes. Done. |
No description provided.