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

Add nimble token for toolbar height #2493

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aagash-ni
Copy link
Contributor

@aagash-ni aagash-ni commented Dec 11, 2024

Pull Request

🤨 Rationale

Added the Nimble token 40px for toolbar height as it is required in the following areas:

  • Rich Text Editor Footer Section: Currently implemented using an internal CSS variable.

image

👩‍💻 Implementation

  • Added 40px height token for toolbar $ni-nimble-toolbar-height.

🧪 Testing

NA

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@aagash-ni aagash-ni marked this pull request as ready for review December 11, 2024 12:09
@@ -0,0 +1,7 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for thinking of trying to single-source this in Nimble. But I have an initial concern: the Nimble toolbar itself would not use this token, so it seems strange to expose it as the "official" toolbar height. My concern with that is that a client would probably expect that they could override the value of this token and have all Nimble toolbars update, but that would not be the case.

The reason the Nimble toolbar wouldn't use this token is that the toolbar sizes itself based on the height of its content plus some vertical padding (32px +2*4px = 40px). I suspect we want to keep that behavior because we may want both shorter and taller toolbars for certain use cases.

Could the use cases you describe follow the same approach and avoid the need for a new token entirely? A downside of that approach is that if we ever change our minds about this behavior (e.g. change the vertical padding) then it would be hard to identify all the places that need to change. But I personally think that's an acceptable risk.

I'm going to mark this as not approved for now but I'm happy to continue the discussion. However, me and the other Nimble reviewers are all going to be OOO for the rest of the year so if you want to continue the conversation it'll have to wait for next year. In the meantime hopefully you can get by with duplicating this value in a few places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants