-
Notifications
You must be signed in to change notification settings - Fork 64
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
[@svelteui/core]: add new drawer component #329
base: main
Are you sure you want to change the base?
Conversation
mineofcode
commented
Mar 5, 2023
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.
Thank you for this contribution! 🙌
Could you run yarn format && yarn lint
?
Also, could you maintain the structure of the PR template? It helps to remember the PR checklist
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.
Could you also include the logic for the focus return, similar to what we have in Modal. It was recently added there, so it would be cool to also have in this component.
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.
The code is clean 👌
Just some small nitpickings and questions
display: 'block', | ||
wordBreak: 'break-word' | ||
}, | ||
Drawer: { |
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.
nitpicking but
Drawer: { | |
drawer: { |
display: 'flex', | ||
alignItems: 'center', | ||
justifyContent: 'space-between', | ||
marginBottom: parseInt(theme.space.md.value), |
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.
why the parseInt?
alignItems: 'center', | ||
justifyContent: 'space-between', | ||
marginBottom: parseInt(theme.space.md.value), | ||
marginRight: -9 |
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.
marginRight: -9 | |
marginRight: '-9px' |
}, | ||
|
||
body: { | ||
maxHeight: overflow === 'inside' ? 'calc(100vh - 185px)' : null, |
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.
any specific reason for the 185px?
$: if (opened) { | ||
setTimeout(() => { | ||
openStyle = position + `: 0px;`; | ||
}, 100); |
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.
Why the 100, specifically?
</div> | ||
{/if} | ||
<div id={bodyId} class={classes.body}> | ||
<slot>Place some content</slot> |
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.
empty slot is better
Hi @masagatech, any updates regarding this? Do you need any help? |
@mineofcode still want to finish this? |