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

Feature: Topics Component Refactor #7376

Merged
merged 45 commits into from
Apr 2, 2024
Merged

Conversation

clmedders
Copy link
Contributor

@clmedders clmedders commented Mar 4, 2024

Summary

Refactors the topics button into a component.

Preview

Link to Preview

Solution

1 - Created partial for topics button to live in, within this file also set logic to render the two topic button variants.
2 - Logic is set based on if tag_count is set within the template and both groups will have the same mark up within the markup
3 - Updated the class naming to follow a more BEM format.

How To Test

  1. Visit preview link and live site and compare to ensure no major visual regressions

Dev Checklist

  • PR has correct labels
  • A11y testing (voice over testing, meets WCAG, run axe tools)
  • Code is formatted properly

Copy link

github-actions bot commented Mar 4, 2024

🔍 Preview in Federalist

@clmedders clmedders marked this pull request as ready for review March 4, 2024 21:35
@clmedders clmedders self-assigned this Mar 4, 2024
- made stylesheets and component name consistent
- improved BEM styles names and format
- improved template logic and added partial component to
  resources/list.html
@nick-mon1 nick-mon1 added Dev: Template Logic Fixes and improvements associated with our static site generator Dev: frontend ideas and issues related to the presentation layer of the site labels Mar 6, 2024
Comment on lines -4 to -8
.resources.section {
.paper {
@include u-bg("blue-warm-5");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Removed .resources.section .paper since it is over-ridden on resources/list.html and sets background color on topics-button-list section which we do not want.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nick-mon1 @clmedders, unresolving the comment as it's useful information for reviewer.

Copy link
Contributor

@nick-mon1 nick-mon1 left a comment

Choose a reason for hiding this comment

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

Made some comments on my changes. Take a look to see if these make sense @clmedders

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

No visual regressions found.

Added comments for improving code quality.


Documenting minor visual change:



Preview →

themes/digital.gov/src/scss/new/_topics-button.scss Outdated Show resolved Hide resolved
themes/digital.gov/src/scss/new/_topics-button.scss Outdated Show resolved Hide resolved
themes/digital.gov/src/scss/new/_topics-button.scss Outdated Show resolved Hide resolved
themes/digital.gov/src/scss/new/_topics-button.scss Outdated Show resolved Hide resolved
nick-mon1
nick-mon1 previously approved these changes Mar 20, 2024
Copy link
Contributor

@nick-mon1 nick-mon1 left a comment

Choose a reason for hiding this comment

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

Added some comments addressing my changes (1492111) and we should be good to go if you don't have any more questions.

Can we confirm on why some topics are displayed as Capital Case versus Sentence case, is this related to a plain language rule? See Application programming interface versus Software Engineering. We can create another ticket if these need to be updated for consistency.

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Please be careful when nesting too much in styles. This can make reading & debugging more difficult.

themes/digital.gov/src/scss/new/_topics-button.scss Outdated Show resolved Hide resolved
themes/digital.gov/src/scss/new/_topics-button.scss Outdated Show resolved Hide resolved
@@ -52,27 +54,30 @@
font-size: font-size("sans", "2xs");
height: fit-content;
margin: auto;
padding-bottom: units("05");
padding-top: units("05");
padding-left: units("05");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to left and right to fix the visual error that came up the circle around the numbers

nick-mon1
nick-mon1 previously approved these changes Mar 22, 2024
Copy link
Contributor

@nick-mon1 nick-mon1 left a comment

Choose a reason for hiding this comment

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

Made a minor update to the styles to prefer units over px-to-rem. 12px token is 1.5. See https://designsystem.digital.gov/design-tokens/spacing-units/

@mejiaj mejiaj requested a review from nick-mon1 March 28, 2024 15:25
@ToniBonittoGSA ToniBonittoGSA merged commit f167ce4 into main Apr 2, 2024
4 checks passed
@ToniBonittoGSA ToniBonittoGSA deleted the cm-topics-button-component branch April 2, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev: frontend ideas and issues related to the presentation layer of the site Dev: Template Logic Fixes and improvements associated with our static site generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants