-
Notifications
You must be signed in to change notification settings - Fork 44
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
AppFrame
component
#1284
AppFrame
component
#1284
Conversation
(re-organized sub-components in “parts” folder for simplicity)
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
aabb4c8
to
2362f2b
Compare
Just wanted to clarify - strict mode is an existing feature of ember's template layer and is the approved future syntax for templates in ember. V2 refers to a new addon layout that's spec'd here: emberjs/rfcs#507. |
// FRAME'S CONTAINERS | ||
|
||
.hds-app-frame__header { | ||
z-index: 7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, why the use of z-index?
One of the things we want to make sure does not happen is that interactive elements could be accidentally hidden behind "sticky" headers or footers.
Some reading:
- WCAG Success Criteria: Focus not obscured
- CSS Technique 43: using CSS margin and scroll-margin to un-obscure content
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're reproducing (with different values) what already happens in production. We need to define a hierarchy of z-index for the top-level content (they will be converted to design tokens in a follow-up task)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current design/implementation, these z-index
values have no effect as all containers have a static position. If one of them changes however (via an override, for example, or a script), having these z-index
values can be a good fallback to make sure they overlap as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, there will be a follow-up PR for handling the z-index
values in a more systematic way (exposing them as design tokens)
FWIW, I think it's a step backward to go to a v1 addon from a v2 addon. I don't really understand why you'd choose to do this when there is a lot of effort being put into getting addons from v1 to v2. Strongly encourage a re-think on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a few nitpicky content things, but otherwise, this looks good!
Co-authored-by: Heather Larsen <hlarsen@hashicorp.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Don't forget the changelog entry
packages/components/tests/integration/components/hds/app-frame/index-test.js
Outdated
Show resolved
Hide resolved
// FRAME'S CONTAINERS | ||
|
||
.hds-app-frame__header { | ||
z-index: 7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current design/implementation, these z-index
values have no effect as all containers have a static position. If one of them changes however (via an override, for example, or a script), having these z-index
values can be a good fallback to make sure they overlap as expected.
e18f593
to
56ebc72
Compare
📌 Summary
This PR is a follow-up of https://github.com/hashicorp/ember-shared-components/pull/3 and is the first of the two components ported-back from Cloud UI (and
ember-shared-components
) to HDS (the other one is #1304).For details about this decision, see the specific RFC: [DS-047] AppFrame / SideNav - From Cloud UI to HDS
🛠️ Detailed description
In this PR I have:
AppFrame
code from theHcAppFrame
implementationv2
tov1
addon formatparts
folder for simplicity, instead of having siblingsAppFrame
component (see preview below)AppFrame
component (see preview below)👉 👉 👉 Previews:
🔗 External links
Jira ticket: https://hashicorp.atlassian.net/browse/HDS-1749
Figma file: this is a layout-only component, no design associated to it
👀 Reviewer's checklist:
💬 Please consider using conventional comments when reviewing this PR.