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

Specialize popup menu classes (with prefix) #703

Closed
Tracked by #1793
smbea opened this issue Nov 21, 2022 · 4 comments · Fixed by #704
Closed
Tracked by #1793

Specialize popup menu classes (with prefix) #703

smbea opened this issue Nov 21, 2022 · 4 comments · Fixed by #704
Assignees

Comments

@smbea
Copy link
Contributor

smbea commented Nov 21, 2022

What should we do?

Investigate if other djs elements maintain specialized prefixed classes (eg: djs-popup-...) at all times, and decide if we should follow this for the popup menu.

Why should we do it?

To have a clear understand of how djs elements should be implemented and to keep consistency with other djs elements.

@smbea smbea added the ready Ready to be worked on label Nov 21, 2022
@smbea smbea self-assigned this Nov 21, 2022
@smbea
Copy link
Contributor Author

smbea commented Nov 21, 2022

After some investigation, I found that djs elements mostly follow the principle of prefixing the classes but there are some exceptions:

Common classes shared by multiple elements: .entry, .open
Some other classes used for a single element:

  • .new-parent
  • .highlighted-entry
  • .separator
  • .two-column
  • .floating

I feel like the exceptions are few also because we haven't added many complex elements yet.

In popup menu, we have some classes that may be more generic and therefore cause more collision, like .name, .title, .results. I think we should keep .entry anyway to be consistent.

To move forward, we can:

  1. Ignore this for now. Like Nico said, we haven't had any complaints yet.
    1.1 This means we integrate it faster but might also mean revisiting this in the future (if needed)
  2. Prefix the popup menu classes added
    2.1. Since this causes more breaking changes, does it mean we need another major release?

cc @nikku @barmac

@nikku
Copy link
Member

nikku commented Nov 21, 2022

Since this causes more breaking changes, does it mean we need another major release?

If we go for this (and follow up soon), let's bake it into a 11.x minor release.

11.x has not been adopted to date.

@nikku
Copy link
Member

nikku commented Nov 21, 2022

You could also mark 11.x as next on npm to be sure.

==> diagram-js@10.0.0 is now latest again.

@smbea
Copy link
Contributor Author

smbea commented Nov 21, 2022

Then I vote we do it now, since we have to do a release anyway. It should be fairly quick to change the classes to djs-popup-....

@smbea smbea changed the title Decide if we should specialize popup menu classes Specialize popup menu classes (with prefix) Nov 21, 2022
smbea added a commit that referenced this issue Nov 21, 2022
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed ready Ready to be worked on labels Nov 21, 2022
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending in progress Currently worked on and removed in progress Currently worked on needs review Review pending labels Nov 21, 2022
smbea added a commit that referenced this issue Nov 21, 2022
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Nov 21, 2022
smbea added a commit that referenced this issue Nov 21, 2022
nikku pushed a commit that referenced this issue Nov 21, 2022
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Nov 21, 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