-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Center InternalDatePicker Clear button #3265
Center InternalDatePicker Clear button #3265
Conversation
CLA
Hello there and welcome to our project! |
|
||
& .menu-item { | ||
height: ${({ theme }) => theme.spacing(8)}; | ||
margin-bottom: ${({ theme }) => theme.spacing(1)}; | ||
margin-top: ${({ theme }) => theme.spacing(1)}; | ||
padding: 0 ${({ theme }) => theme.spacing(2)}; | ||
width: fit-content; | ||
|
||
& > div { |
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.
We avoid styling children through CSS.
Instead you can transform the div itself to a Styled component
Hi @leojalfred, it seems that you're both working on the same issue with @abdul-irfan-k, see other PR : #3273 Would you be ok to work together on this ? We need this to be pixel perfect like in the figma. |
@lucasbordeau I'd be happy to. I can push up necessary changes tomorrow morning. Do you have a specific example in Figma that you want me to reference? I'm looking at the calendar components linked on the repo's README and I don't see a mockup for this. |
@leojalfred Thank you :) |
@charlesBochet I saw that but that doesn't really have much to do with the specifics of this ticket. Also the existing implementation doesn't match the design in Figma, particularly with respect to the padding. I can do my best when it comes to the padding, but it makes more sense to have the spacing for the Clear button line up with the rest of the spacing in existing implementation rather than the design in Figma. |
Also the div that you pointed out in the comment was already a styled component ( I've got a question about using children in styling. It seems like children are used in styling, as in targeting |
@charlesBochet it should be good to go. I changed the justify-content: center that was originally styled on the div to be styled via props in |
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 for the design.
Could you just create the button from scratch and not use the MenuItem component ?
Because MenuItem is used everywhere in the code and refactoring it to handle centered text is out of the scope of this issue.
We can see in storybook for MenuItem, that it doesn't play well with isCentered set to true and right icon buttons.
We'll see how to refactor this MenuItem later.
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 great! Perfect, thank you :)
Addresses #3249 by altering the
InternalDatePicker
Clear button to take the full width. Note that.hoverable-buttons
is now conditionally rendered to prevent issues when centering the items in the button.This is my first open source contribution, please let me know if I can do anything to streamline my contribution.