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

feat: restructure menubar #2 #6411

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

feat: restructure menubar #2 #6411

wants to merge 4 commits into from

Conversation

mejo-
Copy link
Member

@mejo- mejo- commented Sep 16, 2024

📝 Summary

  • refactor(menu): Support several types in a list for isActive check
  • feat(menu): Group block elements in a submenu
  • fix(menu): Move heading left of bold, links and attachments neighbors
  • fix(menu): change headings submenu icon to FormatSize icon
  • Contributes to: Menu bar redesign #2836

🖼️ Screenshots

🏚️ Headings menu before 🏡 Headings menu after
image image
🏚️ Callouts menu before 🏡 Blocks menu after
image image
Screencast
recording1

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

hmm not sure about the block icon, I think it's not too clear 🤔

@juliushaertl
Copy link
Member

I find it quite fitting, also considering the other icons we looked at this week

@mejo-
Copy link
Member Author

mejo- commented Sep 17, 2024

hmm not sure about the block icon, I think it's not too clear 🤔

@marcoambrosini I find it fitting well, given that it shows a box with text inside - which describes text boxes pretty well. Do you have an idea for an alternative icon? And are you against the icon or just not 100% sure?

@mejo- mejo- added 3. to review enhancement New feature or request design Experience, interaction, interface, … and removed 2. developing labels Sep 17, 2024
@max-nextcloud
Copy link
Collaborator

I like it. I'm not sure about the 'no heading' icon with the two T though. If the text is not bold the icon is just deselected and does not change to a thin 'F' either. Same for the lists. If no list type is selected the icon still displays a list and not a 'non-list' state.

@mejo-
Copy link
Member Author

mejo- commented Sep 18, 2024

I'm not sure about the 'no heading' icon with the two T though. If the text is not bold the icon is just deselected and does not change to a thin 'F' either. Same for the lists. If no list type is selected the icon still displays a list and not a 'non-list' state.

I implemented it as it was once decided in #2836. I think the idea was that it's a more generic icon to communicate "text size" than "H1". I don't have a strong opinion on which one communicates better. "H1" might also be hard to understand for non-english speakers. Maybe designers can comment here @marcoambrosini @nimishavijay 😊

@marcoambrosini
Copy link
Member

I find it:

  • too similar to lists > difficult to quickly find
  • Rectangle shape similar to neighbouring icons > difficult to quickly find
  • reminds me of left-alignment icon > possibly confusing

Maybe brackets are good for this case?

Screenshot 2024-09-19 at 09 17 58

So I would go with:
https://fonts.google.com/icons?selected=Material+Symbols+Outlined:data_array:FILL@0;wght@400;GRAD@0;opsz@24&icon.query=parenthe&icon.size=24&icon.color=%235f6368

@juliushaertl
Copy link
Member

Brackets I would find to easy to only associate with a code block, not sure this is a good one for average users as well to refer to as blocks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review design Experience, interaction, interface, … enhancement New feature or request
Projects
Status: 🏗️ In progress
Development

Successfully merging this pull request may close these issues.

4 participants