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

ui: New Confirmation Dialogs #7007

Merged
merged 7 commits into from
Jan 22, 2020
Merged

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Jan 7, 2020

This PR is changes our old inline confirmation dialogs to use a new style in the intention listing only (to provide context/preview).

confirm

Preview Link

Additional work on top of this will include implementing this into our other list views, currently they are completely broken so this PR should not be merged until that work is completed.

We reused our work from the namespace/dc menu here, but we've specifically disabled the keyboard navigation on these dialogs for the moment, we plan on getting this into a release before working further on the keyboard functionality here.

Also included here is a different approach to managing our z-indexing in our list views, this approach massively simplifies our previous approach and means we have to overwrite far less of the methods of the parent component.

Turns out zIndexing remained problematic. But preferably we wanted to keep things simpler than the old approach, although unfortunately we still need to delve into ember-collection to be able to maintain correct zIndexing. The approach is simpler but hackier, but this is preferable to the old approach of duplicating all the code from the ember-collection addon. We did have a quick think as to whether we could make a child class of Grid so it was less hacky but as we can't control the arguments that get passed to formatItemStyle it that wouldn't help us here. The result of all this is in d983820

@johncowen johncowen requested a review from a team January 7, 2020 15:46
@johncowen johncowen added theme/ui Anything related to the UI pr/do-not-merge PR cannot be merged in its current form. labels Jan 7, 2020
@johncowen johncowen force-pushed the feature/ui-reuse-popup-menu branch 2 times, most recently from 81635c5 to b89272b Compare January 15, 2020 14:05
Copy link
Contributor

@kaxcode kaxcode left a comment

Choose a reason for hiding this comment

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

Pulled branch locally to play with the dialog. The z-index is 👍

@johncowen
Copy link
Contributor Author

Yey, thanks @kaxcode ! I'm going to branch off from here to implement the new dialogs in the other areas we need them, once thats done I'll merge here and then finally merge both down to ui-staging.

* ui: Implement new confirmation dialogs in other list views

This includes another amend to the popover-menu in order to allow
mutiple confirmations/subpanels in the same popover menu.

The functionality added here to allow this is likely to change in the
future.
@johncowen
Copy link
Contributor Author

I merged #7080 in here, everything is still passing and I tried it out some more, so I'm gonna merge this down onto ui-staging now 👍

@johncowen johncowen merged commit afafe67 into ui-staging Jan 22, 2020
@johncowen johncowen deleted the feature/ui-reuse-popup-menu branch January 22, 2020 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/do-not-merge PR cannot be merged in its current form. theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants