Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mineofcode
Copy link
Contributor

image

@BeeMargarida BeeMargarida changed the title [@svelteui/core]: [@svelteui/core]: add new drawer component [@svelteui/core]: add new drawer component Mar 5, 2023
@BeeMargarida BeeMargarida added enhancement New feature or request component New or changes to components labels Mar 5, 2023
Copy link
Member

@BeeMargarida BeeMargarida left a 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

Copy link
Member

@BeeMargarida BeeMargarida left a 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.

Copy link
Member

@BeeMargarida BeeMargarida left a 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: {
Copy link
Member

Choose a reason for hiding this comment

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

nitpicking but

Suggested change
Drawer: {
drawer: {

display: 'flex',
alignItems: 'center',
justifyContent: 'space-between',
marginBottom: parseInt(theme.space.md.value),
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
marginRight: -9
marginRight: '-9px'

},

body: {
maxHeight: overflow === 'inside' ? 'calc(100vh - 185px)' : null,
Copy link
Member

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);
Copy link
Member

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>
Copy link
Member

Choose a reason for hiding this comment

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

empty slot is better

@BeeMargarida
Copy link
Member

Hi @masagatech, any updates regarding this? Do you need any help?

@Brisklemonade
Copy link
Collaborator

@mineofcode still want to finish this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component New or changes to components enhancement New feature or request
Projects
Status: Next
Development

Successfully merging this pull request may close these issues.

3 participants