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 section to panel view #1868

Closed

Conversation

giovanniTramonto
Copy link
Contributor

This adds the possibility to wrap panel contents in multiple auto-scrollable sections. So overflow logic is separated from contents.

Changes

  • removed getContentElement function for wrapper consistency
  • added getPanelSection factory to handle overflow of contents (this removed the extraneous styles from ui-form)
  • deferred use of static styles in panel class extensions
  • implement panel sections across the application

@justvanrossum
Copy link
Collaborator

Can you ensure this is this purely a code change, and doesn't change any looks or functionality?

@giovanniTramonto giovanniTramonto force-pushed the add-panel-section branch 3 times, most recently from 0b6c116 to 01b89ff Compare December 17, 2024 19:59
@giovanniTramonto
Copy link
Contributor Author

Yes, double checked.

@ollimeier
Copy link
Collaborator

@justvanrossum I had a look at the branch and I would say mostly it looks good. There is one change, which is consistent now with the other sidebars, but differs in the user experience in compare to our current main branch: the glyphs-search-field scrolls 'away' and I am not sure if we want that. Please see the recording:

Glyph-Search-is-now-different.mp4

It would be an exception. I am not sure, how we want to deal with it.

@justvanrossum
Copy link
Collaborator

the glyphs-search-field scrolls 'away' and I am not sure if we want that

I am very sure we don't want that. So this is an unacceptable change.

I am not sure of the motivation of this PR. What exactly does this improve and why do we need it?

@giovanniTramonto
Copy link
Contributor Author

Thanks @ollimeier for checking. Search scroll is adjusted to prior style.
For a follow up: would be nicer if the scroll bar would appear not inside the glyph list, but outside in the padding space. So glyph content has a bit more space and would be consistent.

@justvanrossum Improvements of this MR:

  • consistency in panel
  • isolate panel overflow css from content (i.e. overflow is out of ui-form)
  • css improvements (i.e. in /panel-related-glyphs.js the height: calc(100% - 2em); // Would be nice to do without the calc is gone)
  • less code

@giovanniTramonto
Copy link
Contributor Author

I guess the changes of this MR are too extensive. See this instead: #1877

@justvanrossum
Copy link
Collaborator

I guess the changes of this MR are too extensive

Not necessarily: I just didn't have time to properly test this. But if you feel your new PR is to be preferred anyway, I'll gladly review that one instead.

@giovanniTramonto
Copy link
Contributor Author

I guess the changes of this MR are too extensive

Not necessarily: I just didn't have time to properly test this. But if you feel your new PR is to be preferred anyway, I'll gladly review that one instead.

Yes, thanks, I feel better with a this small change for now. Even though I guess it would be good to make the panel code more consistent. Maybe in a follow up.

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