Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: horizontal mode #124
base: main
Are you sure you want to change the base?
feat: horizontal mode #124
Changes from 5 commits
5e7eee1
c5504e4
50ed192
55f10ae
cdd8229
7c30d27
eeddcff
b88ba1c
79a6b40
9637fb9
3ae5a7e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think this might be an issue with an element that's made the full height of the window. An example would be a drawer or a sidebar. Upon resizing, hardcoding the height would cause the sidebar to not stay adhered to the window edge.
We might need to use a resizeobserver in order to update the height
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.
Or maybe it should be an option to persist the height when the width is animated and collapsed? Not sure which is better
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.
There is two places where we persist the height:
If we are going to use resizeobserver (observe height) on the element it will interfere with collapase when it is changing the height for animated width.
I think the second option you mentioned is already handled in close method and for me i go for this option, but if you have any suggestions to improve it let me know please ;)
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.
I pulled up the storybook, and it looks like the fixed height is always applied (it would need to be removed in the transitionend handler. I made an example in the example app that shows this quick and dirty sidebar demo.
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.
Good point, i think we should reset height on transitionEnd then I'll check other scenarios
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.
Actually responsive and fixed value of height will work perfectly without saving initial height.
The only issue that we will have is with the auto height, it is changing while animating width, so i think in this case it is up to the user to set the option collapseStyles what do you think ? I tried a solution by adding a setTimeout to detect if height is changed at middle of transition and it works ! but if the default expand is false at the initialization there is no chance to have this information ^^