Skip to content

Conversation

zamoore
Copy link
Contributor

@zamoore zamoore commented Sep 18, 2025

📌 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 a margin: 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 level ApplicationState component. This argument defaults to true and applies the margin: 0 auto; style rule via the hds-application-state--is-auto-centered class. This behavior can be disabled by setting @isAutoCentered to false.

🔗 External links

Jira ticket: HDS-3584


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

📋 PCI review checklist
  • If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
  • If applicable, I've worked with GRC to document the impact of any changes to security controls.
    Examples of changes to controls include access controls, encryption, logging, etc.
  • If applicable, I've worked with GRC to ensure compliance due to a significant change to the in-scope PCI environment.
    Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.

Copy link

vercel bot commented Sep 18, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Updated (UTC)
hds-showcase Ready Ready Preview Sep 24, 2025 6:42pm
hds-website Ready Ready Preview Sep 24, 2025 6:42pm

Copy link
Contributor

@didoo didoo left a 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?

@zamoore
Copy link
Contributor Author

zamoore commented Sep 19, 2025

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. autoCenter sounds good to me. This is similar to what we did for isInline in the icon component, so it has precedent.

@zamoore zamoore force-pushed the main-5.0.0 branch 2 times, most recently from 42e1aaa to 468a8bc Compare September 22, 2025 21:19
@zamoore zamoore force-pushed the zamoore/hds-3584-update-ApplicationState-layout branch from 852efe0 to e904f7d Compare September 22, 2025 21:31
Copy link
Contributor

@didoo didoo left a 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

Comment on lines +7 to +9
`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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`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) {
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-website Content updates to the documentation website packages/components showcase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants