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

Fix README typos, remove next-specific data attributes for components #801

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

randimays
Copy link
Contributor

@randimays randimays commented Oct 29, 2024

Description

Removes data-next-component attributes from components that have them as we won't be needing this data in the markup. This also includes updates to the design system breakpoints that have been done in content-build and vets-website. Some of this code predates those changes so I applied them in this PR. There should be no functionality changes; it's only a name change.

Ticket

Relates to department-of-veterans-affairs/va.gov-cms#19386

Developer Task

Tasks

Preview Give feedback

@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 29, 2024 18:00 Destroyed
@randimays randimays marked this pull request as ready for review October 29, 2024 18:04
@@ -45,7 +45,6 @@ describe('<CollapsiblePanel> with valid data', () => {

test('renders <CollapsiblePanel /> with first panel initially expanded', () => {
render(<CollapsiblePanel {...data} startExpanded={true} />)
const vaAccordion = document.querySelector('va-accordion')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used in this code block so I removed it.

@randimays randimays requested a review from timcosgrove October 29, 2024 19:28
@eselkin
Copy link

eselkin commented Oct 29, 2024

@randimays Should the package.json be brought up to what it is in vets-website if using the current breakpoints stuff?

    "@department-of-veterans-affairs/component-library": "^47.2.0",
    "@department-of-veterans-affairs/css-library": "^0.12.2",
    "@department-of-veterans-affairs/formation": "11.0.26",

Or were they functional in the current ones?

@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 29, 2024 19:47 Destroyed
@randimays randimays force-pushed the update-data-templates branch from 7d5d29b to c5a40dc Compare October 29, 2024 19:51
@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 29, 2024 19:52 Destroyed
@randimays
Copy link
Contributor Author

randimays commented Oct 29, 2024

@eselkin Great callout. I just double checked and they both work, but notably the now-deprecated version isn't applying correctly so we are benefiting from this either way. Screenshots below were taken at 1201px browser width, which is the DST breakpoint for large-screen / desktop-lg

Before

Screenshot 2024-10-29 at 2 55 15 PM Screenshot 2024-10-29 at 2 59 21 PM

After

Screenshot 2024-10-29 at 2 55 27 PM Screenshot 2024-10-29 at 2 59 31 PM

@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 31, 2024 00:33 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 31, 2024 00:45 Destroyed
@va-cms-bot va-cms-bot temporarily deployed to Tugboat October 31, 2024 00:56 Destroyed
@randimays randimays changed the title Fix README typos, add data attributes for components Fix README typos, remove next-specific data attributes for components Oct 31, 2024
@randimays
Copy link
Contributor Author

@timcosgrove Updated the changes to remove the data-next-components instead; I agree that we can rely on browser extensions to identify components and this will just clutter the markup 👍🏻 This is ready for your review again

@randimays
Copy link
Contributor Author

@timcosgrove Bumping for your review when you get a chance please!

@randimays
Copy link
Contributor Author

@timcosgrove Bumping for your review please :)

@timcosgrove timcosgrove merged commit 425d206 into main Nov 13, 2024
12 checks passed
@timcosgrove timcosgrove deleted the update-data-templates branch November 13, 2024 17:16
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.

4 participants