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

Redesign context menu #2596

Merged
merged 17 commits into from Apr 11, 2022
Merged

Redesign context menu #2596

merged 17 commits into from Apr 11, 2022

Conversation

Marc-Spector
Copy link
Collaborator

@Marc-Spector Marc-Spector commented Mar 1, 2022

Closes #2594

Final View

image

@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #2596 (9735fff) into develop (e19777b) will increase coverage by 0.05%.
The diff coverage is 97.05%.

❗ Current head 9735fff differs from pull request most recent head 954f16d. Consider uploading reports for the commit 954f16d to get more accurate results

@@              Coverage Diff              @@
##             develop    #2596      +/-   ##
=============================================
+ Coverage      65.19%   65.25%   +0.05%     
- Complexity      4797     4819      +22     
=============================================
  Files            519      520       +1     
  Lines          19898    19931      +33     
  Branches        1143     1144       +1     
=============================================
+ Hits           12973    13005      +32     
  Misses          6291     6291              
- Partials         634      635       +1     
Impacted Files Coverage Δ
...nt/fx/contextmenu/ChangeUsernameColorMenuItem.java 90.90% <90.90%> (ø)
.../faforever/client/chat/ChatUserItemController.java 93.12% <100.00%> (ø)
...orever/client/fx/contextmenu/AbstractMenuItem.java 100.00% <100.00%> (ø)
...t/fx/contextmenu/CancelActionNotifyMeMenuItem.java 90.90% <100.00%> (+0.90%) ⬆️
...menu/CancelActionRunReplayImmediatelyMenuItem.java 90.90% <100.00%> (+0.90%) ⬆️
...rever/client/fx/contextmenu/CopyLabelMenuItem.java 100.00% <100.00%> (ø)
...er/client/fx/contextmenu/CopyUsernameMenuItem.java 100.00% <100.00%> (ø)
.../client/fx/contextmenu/EditPlayerNoteMenuItem.java 100.00% <100.00%> (ø)
...orever/client/fx/contextmenu/NotifyMeMenuItem.java 90.90% <100.00%> (+0.90%) ⬆️
...er/client/fx/contextmenu/ReportPlayerMenuItem.java 100.00% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e19777b...954f16d. Read the comment docs.

@Marc-Spector

This comment was marked as resolved.

@Marc-Spector

This comment was marked as outdated.

@BlackYps
Copy link
Collaborator

BlackYps commented Mar 2, 2022

Nice Work! I think an exclamation mark would fit better as a report symbol. Right now it looks more like a conversation option

@Marc-Spector
Copy link
Collaborator Author

The icons of the items are updated. See the first message.

@BlackYps
Copy link
Collaborator

BlackYps commented Mar 2, 2022

Isn't that an "i" instead of an "!"?

@Sheikah45
Copy link
Member

The only thing I am not sure about from a UX experience is the now two clicks required to change color and inability to see the color in the menu, although I could be convinced it is worth it

@Marc-Spector
Copy link
Collaborator Author

Marc-Spector commented Mar 3, 2022

inability to see the color in the menu

Are you joking? 😏 It is impossible not to see the color in the menu.

I misunderstood you. I don't think this is the commonly used feature. This is my opinion.

@Marc-Spector
Copy link
Collaborator Author

Isn't that an "i" instead of an "!"?

Oops... My mistake, I will fix it

@Marc-Spector

This comment was marked as outdated.

@Sheikah45
Copy link
Member

Ah I just realized this is introducing a new icon source. Is there any way we could use icons from our current source and method of using the svg paths? The icon svgs are in src/icomoon and the icons are from https://icomoon.io so we could add icons to that set if needed. You can import the json file to see the available ones.

@Marc-Spector
Copy link
Collaborator Author

https://github.com/FAForever/downlords-faf-client/wiki/Adding-Icons
Maybe just add icons to icons.css and use them?

@Sheikah45
Copy link
Member

Sheikah45 commented Mar 3, 2022

FAForever/downlords-faf-client/wiki/Adding-Icons Maybe just add icons to icons.css and use them?

That would work as well. Just want the way we set and store icons to be uniform

@Marc-Spector
Copy link
Collaborator Author

I will continue to work with the PR when #2591 will be merged

@Sheikah45
Copy link
Member

@Marc-Spector did you plan on continuing this?

@Marc-Spector
Copy link
Collaborator Author

Of course. When I have free time and desire.
There are problems with icons. They look bad and scale poorly when used through style classes.

@Marc-Spector
Copy link
Collaborator Author

This problem would have been solved if we knew how to assign the size of the viewBox in css.

@Marc-Spector
Copy link
Collaborator Author

All icons taken from icomoon.
Note that not every menu item contains an icon, because the icons are poorly scaled. Maybe in the future we will find better icons and add them.

@Marc-Spector
Copy link
Collaborator Author

Marc-Spector commented Apr 11, 2022

@Sheikah45 The PR is ready (finished)

@Marc-Spector
Copy link
Collaborator Author

I reverted readme.md file. I don't know why it is displayed in the Files changed tab

@Sheikah45 Sheikah45 merged commit 46e279b into develop Apr 11, 2022
@Sheikah45 Sheikah45 deleted the feature/redesign-context-menu branch April 11, 2022 22:30
mrchris2000 pushed a commit to mrchris2000/downlords-faf-client that referenced this pull request Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the context menu more attractive to the user
4 participants