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

status tags for modular dashboard #723

Merged
merged 5 commits into from
Jan 9, 2025
Merged

status tags for modular dashboard #723

merged 5 commits into from
Jan 9, 2025

Conversation

shayan-roshan
Copy link
Collaborator

Copy link

github-actions bot commented Dec 12, 2024

PR Preview Action v1.5.0
Preview removed because the pull request was closed.
2025-01-09 20:27 UTC

Copy link
Collaborator

@cwolf10 cwolf10 left a comment

Choose a reason for hiding this comment

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

Can you also double check how mobile should be handled? I'm not seeing any conditions for mobile in your styling and imagine overlapping the text is not how the designers would want that handled.
image

&--info {
@include u-bg("info-lighter");
@include u-border("info-light");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you confirm with Shannon that they wanted to use info-light from USWDS? For the buttons, we went with a one-off tweak in the color, so the info-light button won't match this border color.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the description in the story says that the color they want for the border is info-light
Screen Shot 2024-12-12 at 4 53 42 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes but between making that story and merging #718 the designers chose to adjust what hex code was used for the buttons that you are using on those card. If you just use the USWDS color token, the buttons and status border won't match.

@shannoncates-gsa can you confirm whether the info and error statuses should use the USWDS info and error tokens or should they use the adjusted tokens that we used for the buttons?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I confirmed with Shannon and fixed the colors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You've still got info-lighter which was also re-defined to E7F6F8, that intended to be left to the USWDS color?

@include u-border("accent-cool");
}
&--locked {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you include some spacing between the icon and text to match the mockup?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good

@shayan-roshan
Copy link
Collaborator Author

mobile view fixed

@cwolf10
Copy link
Collaborator

cwolf10 commented Dec 18, 2024

Mobile view looks better without the overlap. I am seeing that the borders of the main card and the status tag aren't matching though. The status has got square corners and the card is rounded. Might want to round the corners so that everything lines up.

image

@shayan-roshan
Copy link
Collaborator Author

border radius fixed

@cwolf10
Copy link
Collaborator

cwolf10 commented Dec 23, 2024

Mobile fix looks good. I still see that the info border is using the USWDS color rather than the slight variation we are using for the button. Did Shannon say that was intended to be different?

@cwolf10 cwolf10 merged commit e238947 into master Jan 9, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants