twenty
twenty copied to clipboard
twentyhq
Center InternalDatePicker Clear button
Addresses https://github.com/twentyhq/twenty/issues/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.
CLA
Hello there and welcome to our project! By submitting your Pull Request, you acknowledge that you agree with the terms of our Contributor License Agreement. Although we don't have a dedicated legal counsel, having this kind of agreement can protect us from potential legal issues or patent trolls. Thank you for your understanding.
Generated by :no_entry_sign: dangerJS against 0d1a7c3566de04fee3671adc237b6f3bfc4750d1
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hi @leojalfred, it seems that you're both working on the same issue with @abdul-irfan-k, see other PR : https://github.com/twentyhq/twenty/pull/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 :) The figma is located here: https://www.figma.com/file/xt8O9mFeLl46C5InWwoMrN/Twenty?type=design&node-id=2709-52753&mode=design&t=bTCrlh4rIYbzufiM-0
@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 (StyledMenuItemLeftContent). The reason I used the div in styling is because it avoided the need for prop drilling. I think in order to not use children in the styling I'll need to pass a centering boolean down the chain to that component and use it to add justify-content: center.
I've got a question about using children in styling. It seems like children are used in styling, as in targeting .menu-item under the StyledButtonContainer styles. Should this be refactored as well? Or is there some rule that determines when it's appropriate to use children in styling and when it's not?
@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 StyledMenuItemLeftContent. Per the other PR I changed StyledButtonContainer to extend MenuItem. Per the other PR I also changed the StyledButtonContainer spacing to be the same vertically and horizontally, but to my eye it looks worse.