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

fix: lock-button-tooltip #1768 #1861

Conversation

giovanniTramonto
Copy link
Contributor

  • handle ui-form overflow and padding on :host (to prevent clipping)
  • add form modifier class for selection-info and selection-transformation (keeps flex settings within parent element scope)

Copy link

google-cla bot commented Dec 16, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@ollimeier
Copy link
Collaborator

@justvanrossum I checke out the branch and tested it. I can confirm that it fixes the issue with the tooltip:

Screenshot 2024-12-16 at 20 55 11

@justvanrossum
Copy link
Collaborator

Does adding padding to :host have any downsides?

@ollimeier
Copy link
Collaborator

Does adding padding to :host have any downsides?

I sadly don't have experiences with :host, but I don't read about any downsides. You can simply style :host as any other CSS class.
Please see: https://developer.mozilla.org/en-US/docs/Web/CSS/:host

The following might also be interesting to read:
https://stackoverflow.com/questions/46448385/use-of-host-selector-vs-container-div#answer-46478233

I am wondering if this should be done in other places as well?

@justvanrossum
Copy link
Collaborator

My question wasn't about :host, but about adding the padding to the ui-info itself instead of where it is being used.

I'm curious if there is a difference in appearance before and after:

  • with long axis names in the selection info panel
  • the scroll bar of the selection info panel

@giovanniTramonto
Copy link
Contributor Author

The branch fixes the clipping issue. But there is more. I’d propose a component panel-section that concerns the overflow and wraps any contents inside. It would allow multiple scrollable boxes in the panel. Then overflow does not need to be a part of ui-form anymore.

@ollimeier
Copy link
Collaborator

My question wasn't about :host, but about adding the padding to the ui-info itself instead of where it is being used.

I'm curious if there is a difference in appearance before and after:

  • with long axis names in the selection info panel
  • the scroll bar of the selection info panel

@justvanrossum I don't see any differences. Here are some screenshots in compare with our main branch:
Screenshot 2024-12-17 at 04 09 34
Screenshot 2024-12-17 at 04 08 59
Screenshot 2024-12-17 at 04 08 56
Screenshot 2024-12-17 at 04 09 37

@justvanrossum
Copy link
Collaborator

justvanrossum commented Dec 17, 2024

I don't think we should – at this time – go beyond fixing the issue at hand, though. This looks good, and I will play with it a bit, and then merge.

Btw. please sign that CLA thing....

@justvanrossum justvanrossum merged commit 0c41855 into googlefonts:main Dec 17, 2024
5 checks passed
@giovanniTramonto giovanniTramonto deleted the T-1768-bugifx-lock-button-tooltip branch December 17, 2024 09:44
@giovanniTramonto
Copy link
Contributor Author

giovanniTramonto commented Dec 17, 2024

The branch fixes the clipping issue. But there is more. I’d propose a component panel-section that concerns the overflow and wraps any contents inside. It would allow multiple scrollable boxes in the panel. Then overflow does not need to be a part of ui-form anymore.

Proposal #1868

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