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: preparation for HDS adoption to replace <Modal> #23353

Merged
merged 3 commits into from
Sep 27, 2023

Conversation

hellobontempo
Copy link
Contributor

@hellobontempo hellobontempo commented Sep 27, 2023

Small changes to prepare for implementing the <Hds::Modal> component.

Adds @container to the JsonEditor so copy buttons work inside modals, also moves overflow styling to .body

Page scroll

When a Modal is open, the rest of the page is disabled (via inert). The page scrolling is also disabled by applying overflow: hidden to the <body> element, to make it clear to the user that the underlying elements are not interactive and to avoid confusion. Depending on users’ scroll bar settings, opening a Modal may cause slight layout shifts on the horizontal axis.[docs]

@hellobontempo hellobontempo added this to the 1.16.0-rc1 milestone Sep 27, 2023
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Sep 27, 2023
@hellobontempo hellobontempo marked this pull request as ready for review September 27, 2023 22:12
@@ -10,6 +10,7 @@ import Modifier from 'ember-modifier';

import 'codemirror/addon/edit/matchbrackets';
import 'codemirror/addon/selection/active-line';
import 'codemirror/addon/display/autorefresh';
Copy link
Contributor Author

@hellobontempo hellobontempo Sep 27, 2023

Choose a reason for hiding this comment

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

[docs]

When a modal renders the JsonEditor (ex: with the sample policy template) the content of JsonEditor doesn't render unless the element is focused. Previously we would wait to render the component by wrapping the element in an additional conditional, but this approach no longer works. Now codemirror will autorefresh if passed a @container arg

https://github.com/hashicorp/vault/blob/main/ui/app/templates/components/oidc/scope-form.hbs#L100-L104

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it would be necessary in scenarios other than in a modal where the container value is passed to set the autoRefresh property? I'm guessing no since we haven't run into this until now and we could easily add a dedicated arg for autoRefresh in the future if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - that was my thinking. For now, it seemed okay to have autoRefresh rely on container. But since container could refer to a flyout, dropdown, or maybe even a card - we may want to separate the args in the future!

@hellobontempo hellobontempo changed the title U: HDS adoption replace <Modal> - preparation U: preparation for HDS adoption to replace <Modal> Sep 27, 2023
@github-actions
Copy link

Build Results:
All builds succeeded! ✅

@hellobontempo hellobontempo enabled auto-merge (squash) September 27, 2023 22:29
Copy link
Contributor

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

🤯 Thank you for the comments and investigation on this! LGTM

@hellobontempo hellobontempo merged commit 25985e7 into main Sep 27, 2023
98 of 106 checks passed
@hellobontempo hellobontempo deleted the ui/VAULT-20429/prepare-for-hds-modal branch September 27, 2023 23:21
@hellobontempo hellobontempo changed the title U: preparation for HDS adoption to replace <Modal> Ui: preparation for HDS adoption to replace <Modal> Sep 27, 2023
Copy link
Contributor

@zofskeez zofskeez left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

Comment on lines +30 to +33
* * REQUIRED if rendering within a modal *
* @container gives context for the <Hd::Copy::Button> and sets autoRefresh=true so JsonEditor renders content (without this property @value only renders if editor is focused)
* @param {string} [container] - Selector string or element object of containing element, set the focused element as the container value. This is for the Hds::Copy::Button and to set autoRefresh=true so content renders https://hds-website-hashicorp.vercel.app/components/copy/button?tab=code
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice thanks for documenting this!

@@ -10,6 +10,7 @@ import Modifier from 'ember-modifier';

import 'codemirror/addon/edit/matchbrackets';
import 'codemirror/addon/selection/active-line';
import 'codemirror/addon/display/autorefresh';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it would be necessary in scenarios other than in a modal where the container value is passed to set the autoRefresh property? I'm guessing no since we haven't run into this until now and we could easily add a dedicated arg for autoRefresh in the future if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed pr/no-changelog ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants