-
Notifications
You must be signed in to change notification settings - Fork 51
ApplicationState
- Remove center alignment rule
#3205
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
base: main-5.0.0
Are you sure you want to change the base?
ApplicationState
- Remove center alignment rule
#3205
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Now that we have removed the self-alignment, probably we should update the showcase to have two variants (default + center-aligned) for some of the cases (eg. when it's in a container) because I think in most of the cases, the empty state will be visually centered (in a page, or in a table, or in a card). As far as I remember, the request came from Atlas/TFC and was an exception.
Which leads me to this consideration: what if introducing this change as is now (which is definitely breaking, will impact a lot of places, and can't be codemoded but will require a lot of manual checks) we define a new argument - @hasMarginAuto
or @autoCenter
or or something similar - that by default is true
(so non-breaking) but that consumers can turn off if/when needed?
I really like the idea of introducing a new property. |
7c0bc36
to
81a4cea
Compare
1f56786
to
9c1aec5
Compare
42e1aaa
to
468a8bc
Compare
852efe0
to
e904f7d
Compare
e904f7d
to
86172ab
Compare
81d8152
to
8c88fcd
Compare
86172ab
to
a301b5e
Compare
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.
left a couple of comments/suggestions
`ApplicationState` - Removed the opinionated `margin: 0 auto;` rule from the component's root element to delegate horizontal alignment control to the consumer. | ||
|
||
- Added the new `@isAutoCentered` argument (which defaults to `true`) to preserve the existing centering behavior while allowing consumers to disable it when needed. |
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.
`ApplicationState` - Removed the opinionated `margin: 0 auto;` rule from the component's root element to delegate horizontal alignment control to the consumer. | |
- Added the new `@isAutoCentered` argument (which defaults to `true`) to preserve the existing centering behavior while allowing consumers to disable it when needed. | |
`ApplicationState` - Replaced the default opinionated `margin: 0 auto;` rule from the component's root element and replaced it with a new `@isAutoCentered` argument (which defaults to `true, to preserve the existing centering behavior) to delegate the horizontal alignment control to the consumers, allowing them to disable it when needed. |
.hasClass('hds-application-state--align-center'); | ||
}); | ||
|
||
test('it should have the correct class when isAutoCentered is true', async function (assert) { |
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.
[thought] I am not convinced of this test: usually the way we check things like this is via two different tests:
- one test that checks the default behavior (without setting any argument value, relying on its default)
- one test that checks the alternative behaviour, setting the argument that controls the behaviour
📌 Summary
If merged, this PR removed a style rule that center-aligns the
ApplicationState
component.🛠️ Detailed description
The current implementation of the
ApplicationState
component has amargin: 0 auto;
rule on the root element (.hds-application-state
). This forces a center alignment of the component, which consumers need to override.In order to make adoption of this change as smooth as possible, we have added a new
@isAutoCentered
attribute to the root levelApplicationState
component. This argument defaults totrue
and applies themargin: 0 auto;
style rule via thehds-application-state--is-auto-centered
class. This behavior can be disabled by setting@isAutoCentered
tofalse
.🔗 External links
Jira ticket: HDS-3584
👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.
📋 PCI review checklist
Examples of changes to controls include access controls, encryption, logging, etc.
Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.