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

Added EuiOverlayMask directly to EuiModal #4480

Merged
merged 9 commits into from
Feb 5, 2021
Merged

Conversation

akashgp09
Copy link
Contributor

Summary

This PR Fixes #4478
Added EuiOverlayMask directly to EuiModal

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
    - [ ] Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

Signed-off-by: Akash Gupta <[email protected]>
@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cchaos
Copy link
Contributor

cchaos commented Feb 3, 2021

😆 Well this was an oversight on my part during #4421 which added the overlay mask into the EuiConfirmModal component, but now that this PR adds the overlay mask directly to EuiModal, EuiConfirmModal which imports EuiModal now renders 2 mask. 🤦 ...

@akashgp09 Could you remove the overlay mask now from EuiConfirmModal? And just do a quick search through all the src-docs/ to make sure no other modal instances are wrapped with an overlay mask?

@akashgp09
Copy link
Contributor Author

@cchaos i have made the requested changes ;)

@cchaos
Copy link
Contributor

cchaos commented Feb 3, 2021

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4480/

@cchaos
Copy link
Contributor

cchaos commented Feb 5, 2021

Jenkins, test this

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks @akashgp09 ! There was still one more instance, inside EuiMarkdownEditor that needed fixing. It was a specific use case where the fix was to present an actual "Close" button. I also added some wording to the docs as well as cleaned up some of the guidelines page. When this passes CI, I'll merge.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4480/

@cchaos
Copy link
Contributor

cchaos commented Feb 5, 2021

Flakey tooltip test.

Jenkins, test this

@cchaos
Copy link
Contributor

cchaos commented Feb 5, 2021

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4480/

@cchaos
Copy link
Contributor

cchaos commented Feb 5, 2021

Flakey context menu test... ☹️
Jenkins, test this

@cchaos
Copy link
Contributor

cchaos commented Feb 5, 2021

Testing my patience, Jenkins, test this please!

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4480/

@cchaos cchaos merged commit 5e75993 into elastic:master Feb 5, 2021
andrew-goldstein added a commit to andrew-goldstein/kibana that referenced this pull request Mar 1, 2021
…emoving the EuiOverlayMask

Fixes [this issue](elastic#92798), introduced when [the EUI modal implementation changed](elastic/eui#4480), such that it's no longer necessary to wrap modals in an `EuiOverlayMask`. The mask is now built-in to `EuiModal`.

The change above became effective throughout Kibana when it was upgraded to use a newer version of EUI via [this commit on Feb 16](elastic@8126488#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519).

This PR resolves the issue by removing the `EuiOverlayMask` around the `Customize Event Renderers modal`, shown in the `After` screenshot below:

### Before

![before](https://user-images.githubusercontent.com/59917825/109154007-b2e23880-7793-11eb-83bb-4774df77c5d6.png)

### After

![after](https://user-images.githubusercontent.com/4459398/109561954-0c4fad80-7a9b-11eb-9283-51d50ec8ea26.png)

### Desk testing

Desk-tested on a 16" 2019 MBP, and on the desktop with the following browser versions:

- Chrome `88.0.4324.192`
- Firefox `86.0`
- Safari `14.0.3`
kqualters-elastic pushed a commit to elastic/kibana that referenced this pull request Mar 2, 2021
…ving the EuiOverlayMask (#93150)

* ## [Security Solution] Fixes the Customize Event Renderers modal by removing the EuiOverlayMask

Fixes [this issue](#92798), introduced when [the EUI modal implementation changed](elastic/eui#4480), such that it's no longer necessary to wrap modals in an `EuiOverlayMask`. The mask is now built-in to `EuiModal`.

The change above became effective throughout Kibana when it was upgraded to use a newer version of EUI via [this commit on Feb 16](8126488#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519).

This PR resolves the issue by removing the `EuiOverlayMask` around the `Customize Event Renderers modal`, shown in the `After` screenshot below:

### Before

![before](https://user-images.githubusercontent.com/59917825/109154007-b2e23880-7793-11eb-83bb-4774df77c5d6.png)

### After

![after](https://user-images.githubusercontent.com/4459398/109561954-0c4fad80-7a9b-11eb-9283-51d50ec8ea26.png)

### Desk testing

Desk-tested on a 16" 2019 MBP, and on the desktop with the following browser versions:

- Chrome `88.0.4324.192`
- Firefox `86.0`
- Safari `14.0.3`

* - force precommit git hooks to run

Co-authored-by: Kibana Machine <[email protected]>
kqualters-elastic pushed a commit to kqualters-elastic/kibana that referenced this pull request Mar 2, 2021
…ving the EuiOverlayMask (elastic#93150)

* ## [Security Solution] Fixes the Customize Event Renderers modal by removing the EuiOverlayMask

Fixes [this issue](elastic#92798), introduced when [the EUI modal implementation changed](elastic/eui#4480), such that it's no longer necessary to wrap modals in an `EuiOverlayMask`. The mask is now built-in to `EuiModal`.

The change above became effective throughout Kibana when it was upgraded to use a newer version of EUI via [this commit on Feb 16](elastic@8126488#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519).

This PR resolves the issue by removing the `EuiOverlayMask` around the `Customize Event Renderers modal`, shown in the `After` screenshot below:

### Before

![before](https://user-images.githubusercontent.com/59917825/109154007-b2e23880-7793-11eb-83bb-4774df77c5d6.png)

### After

![after](https://user-images.githubusercontent.com/4459398/109561954-0c4fad80-7a9b-11eb-9283-51d50ec8ea26.png)

### Desk testing

Desk-tested on a 16" 2019 MBP, and on the desktop with the following browser versions:

- Chrome `88.0.4324.192`
- Firefox `86.0`
- Safari `14.0.3`

* - force precommit git hooks to run

Co-authored-by: Kibana Machine <[email protected]>
kqualters-elastic pushed a commit to kqualters-elastic/kibana that referenced this pull request Mar 2, 2021
…ving the EuiOverlayMask (elastic#93150)

* ## [Security Solution] Fixes the Customize Event Renderers modal by removing the EuiOverlayMask

Fixes [this issue](elastic#92798), introduced when [the EUI modal implementation changed](elastic/eui#4480), such that it's no longer necessary to wrap modals in an `EuiOverlayMask`. The mask is now built-in to `EuiModal`.

The change above became effective throughout Kibana when it was upgraded to use a newer version of EUI via [this commit on Feb 16](elastic@8126488#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519).

This PR resolves the issue by removing the `EuiOverlayMask` around the `Customize Event Renderers modal`, shown in the `After` screenshot below:

### Before

![before](https://user-images.githubusercontent.com/59917825/109154007-b2e23880-7793-11eb-83bb-4774df77c5d6.png)

### After

![after](https://user-images.githubusercontent.com/4459398/109561954-0c4fad80-7a9b-11eb-9283-51d50ec8ea26.png)

### Desk testing

Desk-tested on a 16" 2019 MBP, and on the desktop with the following browser versions:

- Chrome `88.0.4324.192`
- Firefox `86.0`
- Safari `14.0.3`

* - force precommit git hooks to run

Co-authored-by: Kibana Machine <[email protected]>
kqualters-elastic added a commit to elastic/kibana that referenced this pull request Mar 2, 2021
…ving the EuiOverlayMask (#93150) (#93213)

* ## [Security Solution] Fixes the Customize Event Renderers modal by removing the EuiOverlayMask

Fixes [this issue](#92798), introduced when [the EUI modal implementation changed](elastic/eui#4480), such that it's no longer necessary to wrap modals in an `EuiOverlayMask`. The mask is now built-in to `EuiModal`.

The change above became effective throughout Kibana when it was upgraded to use a newer version of EUI via [this commit on Feb 16](8126488#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519).

This PR resolves the issue by removing the `EuiOverlayMask` around the `Customize Event Renderers modal`, shown in the `After` screenshot below:

### Before

![before](https://user-images.githubusercontent.com/59917825/109154007-b2e23880-7793-11eb-83bb-4774df77c5d6.png)

### After

![after](https://user-images.githubusercontent.com/4459398/109561954-0c4fad80-7a9b-11eb-9283-51d50ec8ea26.png)

### Desk testing

Desk-tested on a 16" 2019 MBP, and on the desktop with the following browser versions:

- Chrome `88.0.4324.192`
- Firefox `86.0`
- Safari `14.0.3`

* - force precommit git hooks to run

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Andrew Goldstein <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
kqualters-elastic added a commit to elastic/kibana that referenced this pull request Mar 2, 2021
…ving the EuiOverlayMask (#93150) (#93215)

* ## [Security Solution] Fixes the Customize Event Renderers modal by removing the EuiOverlayMask

Fixes [this issue](#92798), introduced when [the EUI modal implementation changed](elastic/eui#4480), such that it's no longer necessary to wrap modals in an `EuiOverlayMask`. The mask is now built-in to `EuiModal`.

The change above became effective throughout Kibana when it was upgraded to use a newer version of EUI via [this commit on Feb 16](8126488#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519).

This PR resolves the issue by removing the `EuiOverlayMask` around the `Customize Event Renderers modal`, shown in the `After` screenshot below:

### Before

![before](https://user-images.githubusercontent.com/59917825/109154007-b2e23880-7793-11eb-83bb-4774df77c5d6.png)

### After

![after](https://user-images.githubusercontent.com/4459398/109561954-0c4fad80-7a9b-11eb-9283-51d50ec8ea26.png)

### Desk testing

Desk-tested on a 16" 2019 MBP, and on the desktop with the following browser versions:

- Chrome `88.0.4324.192`
- Firefox `86.0`
- Safari `14.0.3`

* - force precommit git hooks to run

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Andrew Goldstein <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
@diegozubieta95 diegozubieta95 mentioned this pull request Mar 18, 2022
1 task
@iamandrewluca
Copy link

Hey! Can someone confirm if this should’ve been a breaking change, but was released as minor by mistake? 👀

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.

[EuiModal] Add EuiOverlayMask directly
4 participants