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 concept for modal windows #1686

Merged
merged 8 commits into from
Feb 11, 2020

Conversation

barmac
Copy link
Contributor

@barmac barmac commented Feb 10, 2020

Summary of changes:

  • Close button is now focusable.
  • Close by background click is removed.
  • Modal has a fixed width.
  • Header, body and footer are now visibly separated.
  • Modal keeps responsiveness for smaller screens.
  • In case of content being higher than maximum height,
    modal body is scrollable.

Closes #1681
Closes #1575

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Feb 10, 2020
@barmac
Copy link
Contributor Author

barmac commented Feb 10, 2020

Umm I missed the close icon hover/active style. Let me fix that.

@barmac barmac changed the title Implement concept for modal windows WIP Implement concept for modal windows Feb 10, 2020
* Header, body and footer are now visibly separated.
* Modal keeps responsiveness for smaller screens.
* In case of content being higher than maximum height,
  modal body is scrollable.

Closes #1681
@barmac barmac force-pushed the 1681-implement-concept-for-modal-windows branch from f403f7b to eb0cefa Compare February 10, 2020 12:32
@barmac barmac changed the title WIP Implement concept for modal windows Implement concept for modal windows Feb 10, 2020
@barmac
Copy link
Contributor Author

barmac commented Feb 10, 2020

@nikku @nazlikaya Ready for review

@nikku
Copy link
Member

nikku commented Feb 11, 2020

Some visual impressions:

image

image

image

@nikku
Copy link
Member

nikku commented Feb 11, 2020

Looks good from the feel perspective.

@nikku
Copy link
Member

nikku commented Feb 11, 2020

Added two minor fixes on top.

@philippfromme
Copy link
Contributor

Looking goood! 👍

@barmac
Copy link
Contributor Author

barmac commented Feb 11, 2020

Noticed: missing X button for some modals

@barmac
Copy link
Contributor Author

barmac commented Feb 11, 2020

Screenshot 2020-02-11 at 10 05 02

This needs to be clarified. Not sure what could be the meaning of X button in case of privacy preferences.

@nikku
Copy link
Member

nikku commented Feb 11, 2020

This needs to be clarified. Not sure what could be the meaning of X button in case of privacy preferences.

This is by design. The user may not close the dialog via ESC or close button. Instead, she has to explicitly confirm what is written there (provide consent).

@barmac
Copy link
Contributor Author

barmac commented Feb 11, 2020

This needs to be clarified. Not sure what could be the meaning of X button in case of privacy preferences.

This is by design. The user may not close the dialog via ESC or close button. Instead, she has to explicitly confirm what is written there (provide consent).

Exactly.

@nazlikaya
Copy link

nazlikaya commented Feb 11, 2020

Enter needs to trigger the Primary button, only if the primary button is on focus. Other than that 👍

@barmac
Copy link
Contributor Author

barmac commented Feb 11, 2020

Enter needs to trigger the Primary button, only if the primary button is on focus.

That's how it works right now, right?

@barmac
Copy link
Contributor Author

barmac commented Feb 11, 2020

I added X button for Privacy Preferences when they are already saved somehow (so not for the first startup modal).

@merge-me merge-me bot merged commit 187404f into develop Feb 11, 2020
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Feb 11, 2020
@delete-merged-branch delete-merged-branch bot deleted the 1681-implement-concept-for-modal-windows branch February 11, 2020 15:30
@nikku
Copy link
Member

nikku commented Feb 11, 2020

🍰

@barmac barmac mentioned this pull request Feb 11, 2020
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement concept for Modal windows Modal Inaccessible on Smaller Screens
4 participants