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

Implement popupMenu with improved UI #686

Closed
4 tasks done
Tracked by #1793
smbea opened this issue Oct 31, 2022 · 6 comments · Fixed by #687
Closed
4 tasks done
Tracked by #1793

Implement popupMenu with improved UI #686

smbea opened this issue Oct 31, 2022 · 6 comments · Fixed by #687
Assignees

Comments

@smbea
Copy link
Contributor

smbea commented Oct 31, 2022

What should we do?
We want to replace the old popup UI menu with a new and improved UI, which includes searching.

To do:

  • Integrate diagram-js-ui
  • Implement new UI and replace old popupMenu
    • Have backwards compatibility
    • Improve popup menu foundations
      • Allow grouping, descriptions, documentation url
  • Prioritize and fix most pressing known issues
    • Keyboard accessible
    • Popup menu container within canvas

Initial implementation concluded by #687

Identified bugs/improvements:

Why should we do it?
Preparation to support append anything - see bpmn-io/bpmn-js#1627


Child of bpmn-io/bpmn-js#1627

@smbea
Copy link
Contributor Author

smbea commented Oct 31, 2022

My working branch for this is: https://github.com/bpmn-io/diagram-js/tree/replace-popup-menu

Some properties are customizable (to be passed on by bpmn-js) so they won’t appear at the moment but they are tested:

  • menu title
  • if searchable (default yes if more than 5 entries)
  • custom min-width (width is defined by width of entries, so it varies with searching)

@smbea smbea self-assigned this Oct 31, 2022
@smbea smbea added the ready Ready to be worked on label Oct 31, 2022
@marstamm
Copy link
Contributor

I tested out the current branch of bpmn-js (commit (bpmn-io/bpmn-js@d6a7c0d)),

Overall, it works great. I'll add all my observations here, they might double what is already planned (e.g. min width, according to https://camunda.slack.com/archives/GP70M0J6M/p1667212511108699).

min-width

  • The search box is getting while filtering, which can be confusing. We should consider adding min-width
    Recording 2022-10-31 at 15 34 40 (1)

  • Should we add an empty state, when filtering does not match anything? This might just be because of the missing min-width as well
    image

Highlights

When I have header items and hover over them, multiple elements are highlighted (Multi-Instance and Replace Item). I think this can be confusing and we should only highlight the focused item
image

Search

When the search bar is not visible (< 5 Items), I can still type to search. We should disable this, as there is no visual feedback of what's happening. Here, an empty state ("no Items found") might also help.

Recording 2022-10-31 at 15 44 13

Alternative here: always have a visible search bar

@smbea
Copy link
Contributor Author

smbea commented Oct 31, 2022

RE @marstamm

Regarding the min-width:
This is a tricky one because we need to think of this as as not just the context pad replace menu but also any popup menu. And the min width that makes sense for replace might not be the same for other menus. For example, if we added a min-width that makes sense for replace, it would look like this in align and distribute:
Screenshot 2022-10-31 at 15 49 15

This is why I added a small min-width of 120px that works for all and the possibility to pass on a minWidth as a prop. If present, then it will be applied. This would be added when popup menu is opened (in bpmn-js or wtv): popupMenu.open(..., menuOptions: {title, search, minWidth})

Regarding the empty state and highlighted items:
Makes sense to me! Will look these over

Regarding the search:
The reason for removing is that it may look unnecessary in some cases (like align):
Screenshot 2022-10-31 at 15 48 35

It is always a possibility but I will look into blocking the search feature when the search bar is not displayed.

@marstamm
Copy link
Contributor

Good point regarding the min-width. I like the option to make it configurable, so it works for both cases

smbea added a commit that referenced this issue Nov 1, 2022
smbea added a commit that referenced this issue Nov 2, 2022
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending in progress Currently worked on and removed ready Ready to be worked on needs review Review pending in progress Currently worked on labels Nov 2, 2022
@smbea
Copy link
Contributor Author

smbea commented Nov 3, 2022

I fixed the remaining points (keeping the min width config to be added through ‘popup.open’ args), and opened #687

@smbea smbea added the in progress Currently worked on label Nov 8, 2022 — with bpmn-io-tasks
@smbea smbea removed the needs review Review pending label Nov 8, 2022
smbea added a commit that referenced this issue Nov 8, 2022
@smbea smbea added the needs review Review pending label Nov 8, 2022 — with bpmn-io-tasks
@smbea smbea removed the in progress Currently worked on label Nov 8, 2022
smbea added a commit that referenced this issue Nov 8, 2022
smbea added a commit that referenced this issue Nov 8, 2022
@smbea
Copy link
Contributor Author

smbea commented Nov 8, 2022

I decided to replace the configurable option minWidth for width because not having a set width was causing some issues, for example:

Screen.Recording.2022-11-08.at.21.49.03.mov

This way the width can be configured when/how it is needed by each menu.

nikku pushed a commit that referenced this issue Nov 10, 2022
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Nov 11, 2022
smbea added a commit that referenced this issue Nov 11, 2022
nikku pushed a commit that referenced this issue Nov 14, 2022
nikku pushed a commit that referenced this issue Nov 14, 2022
nikku pushed a commit that referenced this issue Nov 14, 2022
nikku pushed a commit that referenced this issue Nov 14, 2022
smbea added a commit that referenced this issue Nov 15, 2022
smbea added a commit that referenced this issue Nov 15, 2022
smbea added a commit that referenced this issue Nov 15, 2022
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label Nov 15, 2022
@smbea smbea reopened this Nov 22, 2022
@smbea smbea added the ready Ready to be worked on label Nov 22, 2022
@smbea smbea closed this as completed Nov 22, 2022
@bpmn-io-tasks bpmn-io-tasks bot removed the ready Ready to be worked on label Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants