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

fix(overlay): change how containers are hidden #14167

Merged
merged 11 commits into from
Apr 30, 2024

Conversation

simeonoff
Copy link
Collaborator

@simeonoff simeonoff commented Apr 25, 2024

Closes #14030
Closes #14057

Additional information (check all that apply):

  • Bug fix
  • New functionality
  • Documentation
  • Demos
  • CI/CD

Checklist:

  • All relevant tags have been applied to this PR
  • This PR includes unit tests covering all the new code (test guidelines)
  • This PR includes API docs for newly added methods/properties (api docs guidelines)
  • This PR includes feature/README.MD updates for the feature docs
  • This PR includes general feature table updates in the root README.MD
  • This PR includes CHANGELOG.MD updates for newly added functionality
  • This PR contains breaking changes
  • This PR includes ng update migrations for the breaking changes (migrations guidelines)
  • This PR includes behavioral changes and the feature specification has been updated with them

@desig9stein desig9stein added 👀 status: in-review Issue is currently being reviewed 💥 status: in-test PRs currently being tested and removed ❌ status: awaiting-test PRs awaiting manual verification labels Apr 25, 2024
desig9stein
desig9stein previously approved these changes Apr 25, 2024
@desig9stein desig9stein added ✅ status: verified Applies to PRs that have passed manual verification and removed 👀 status: in-review Issue is currently being reviewed 💥 status: in-test PRs currently being tested labels Apr 25, 2024
@pmoleri
Copy link

pmoleri commented Apr 25, 2024

@simeonoff I think there should be and explanation on why it's failing on Safari and why the workaround works. If not in the code at least here in the PR description.

@simeonoff
Copy link
Collaborator Author

@simeonoff I think there should be and explanation on why it's failing on Safari and why the workaround works. If not in the code at least here in the PR description.

I couldn't find the related issue. It has something to do with changing the display property of an element back to display: none. I fail to see why this is important to the end-user!? You should only care if the directive works as expected or not, am I wrong?

@pmoleri
Copy link

pmoleri commented Apr 26, 2024

I fail to see why this is important to the end-user!? You should only care if the directive works as expected or not, am I wrong?

No, I don't think it's important for the end-user. I think it's importante for future maintainance, so someone else will have at least this bit of useful information when tracking back the change to this PR:

I couldn't find the related issue. It has something to do with changing the display property of an element back to display: none.

desig9stein
desig9stein previously approved these changes Apr 29, 2024
@simeonoff simeonoff changed the base branch from master to 17.2.x April 30, 2024 07:58
@simeonoff simeonoff dismissed desig9stein’s stale review April 30, 2024 07:58

The base branch was changed.

@simeonoff simeonoff merged commit 0814c80 into 17.2.x Apr 30, 2024
3 of 4 checks passed
@simeonoff simeonoff deleted the simeonoff/fix-14030-master branch April 30, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
overlay ✨ themes version: 17.2.x ✅ status: verified Applies to PRs that have passed manual verification
Projects
None yet
4 participants