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

Integrate new popup menu #1776

Merged
merged 5 commits into from
Nov 24, 2022
Merged

Integrate new popup menu #1776

merged 5 commits into from
Nov 24, 2022

Conversation

smbea
Copy link
Contributor

@smbea smbea commented Nov 17, 2022

Closes #1775

This is targeting diagram-js@develop since this bug fix was added after the alpha release. My idea is to pre-approve this to make sure the new popup menu is working as expected so we can have the stable release of diagram-js, and later integrate it here.

Integrate diagram-js@11.0.0 diagram-js@11.1.0

Following the title discussion in the issue, I decided not to add the align title for now.

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Nov 17, 2022
@smbea smbea marked this pull request as draft November 17, 2022 14:36
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Nov 17, 2022
@smbea
Copy link
Contributor Author

smbea commented Nov 18, 2022

I will now focus on releasing diagram-js, and then get back to this

@smbea smbea force-pushed the 1775-integrate-new-popup-menu branch from 2d89383 to 83abcbe Compare November 18, 2022 13:32
@smbea smbea marked this pull request as ready for review November 18, 2022 13:40
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Nov 18, 2022
@smbea smbea requested review from a team, philippfromme, barmac and nikku and removed request for a team November 18, 2022 13:41
@smbea
Copy link
Contributor Author

smbea commented Nov 18, 2022

Ready for a final look

@smbea
Copy link
Contributor Author

smbea commented Nov 21, 2022

@nikku Could you have another look at this?

@barmac
Copy link
Member

barmac commented Nov 21, 2022

I am looking into this right now.

@barmac
Copy link
Member

barmac commented Nov 21, 2022

Screen.Recording.2022-11-21.at.09.56.30.mov

Arrow keys don't work in the menu. Is this expected?

@barmac
Copy link
Member

barmac commented Nov 21, 2022

Screen.Recording.2022-11-21.at.09.58.51.mov

Second click on the input closes popup menu though it shouldn't.

@smbea
Copy link
Contributor Author

smbea commented Nov 21, 2022

I created these 2 issues: bpmn-io/diagram-js#701, bpmn-io/diagram-js#702.

@nikku
Copy link
Member

nikku commented Nov 21, 2022

What @barmac reported earlier (mouse vanished, still no keyboard selection) looks like a bug, not a browser quirk 🙈. Fixed behavior showcased here:

capture Y1J0hd_optimized

@nikku
Copy link
Member

nikku commented Nov 21, 2022

Some work in progress fixing issues is pushed at bpmn-io/diagram-js#706.

@smbea smbea force-pushed the 1775-integrate-new-popup-menu branch from 83abcbe to 26fae7e Compare November 24, 2022 09:42
@smbea
Copy link
Contributor Author

smbea commented Nov 24, 2022

diagram-js version has been updated here. I also gave it a final test and didn't find any other issues @nikku @barmac

@nikku nikku force-pushed the 1775-integrate-new-popup-menu branch from 26fae7e to 05c97b8 Compare November 24, 2022 09:56
@nikku
Copy link
Member

nikku commented Nov 24, 2022

Adjusted topic on these commits.

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

User testing this I found the positioning of the replace menu to be off (random place from what it looks like), cf. screens below.

Not sure if this is something we want to adjust, or if this is the last bug in diagram-js@11.

Reference

capture 5cKu8K_optimized

Actual (this PR)

capture uWMYEX_optimized

@smbea smbea force-pushed the 1775-integrate-new-popup-menu branch from 37d1113 to e0230ca Compare November 24, 2022 11:20
@smbea
Copy link
Contributor Author

smbea commented Nov 24, 2022

I think it was missing adjustments here (following the popup menu being pulled out of the canvas). Could you check with my last commit?

Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

LGTM

Screen.Recording.2022-11-24.at.13.32.29.mov

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Looks good to me, too 🏅

@nikku nikku merged commit 93b5f23 into develop Nov 24, 2022
@nikku nikku deleted the 1775-integrate-new-popup-menu branch November 24, 2022 12:41
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Nov 24, 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 this pull request may close these issues.

Integrate new poupMenu
3 participants