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

Add PR diff comments in order #4523

Merged
merged 1 commit into from
Dec 1, 2023
Merged

Add PR diff comments in order #4523

merged 1 commit into from
Dec 1, 2023

Conversation

colinrotherham
Copy link
Contributor

This PR addresses one of the review comments in #4511 (review)

Posting GitHub diff comments in order

We previously used Promise.allSettled() but the least useful "Other changes" comment often comes first

  • I wonder if it's worth rethinking the grouping down the line - feels slightly strange to see CSS and SCSS changes split up on Fix header product name wrapping onto new line #4498 , especially since they directly relate to each other. That could be a whole spiral of work though.

This PR now adds comments in the order they're configured

Copy link

github-actions bot commented Dec 1, 2023

📋 Stats

File sizes

File Size
dist/govuk-frontend-development.min.css 113.65 KiB
dist/govuk-frontend-development.min.js 38.28 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 77.78 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 73.09 KiB
packages/govuk-frontend/dist/govuk/all.mjs 3.86 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 113.64 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 38.27 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.38 KiB

Modules

File Size
all.mjs 69.41 KiB
components/accordion/accordion.mjs 21.67 KiB
components/button/button.mjs 4.7 KiB
components/character-count/character-count.mjs 21.24 KiB
components/checkboxes/checkboxes.mjs 5.83 KiB
components/error-summary/error-summary.mjs 6.09 KiB
components/exit-this-page/exit-this-page.mjs 16.08 KiB
components/header/header.mjs 3.9 KiB
components/notification-banner/notification-banner.mjs 4.54 KiB
components/radios/radios.mjs 4.83 KiB
components/skip-link/skip-link.mjs 3.81 KiB
components/tabs/tabs.mjs 9.66 KiB

View stats and visualisations on the review app


Action run for ed74d5c

Copy link

github-actions bot commented Dec 1, 2023

JavaScript changes to npm package

No changes found.

Action run for ed74d5c

Copy link

github-actions bot commented Dec 1, 2023

Stylesheets changes to npm package

No changes found.

Action run for ed74d5c

Copy link

github-actions bot commented Dec 1, 2023

Other changes to npm package

No changes found.

Action run for ed74d5c

Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Neat! Thanks for splitting 😊 ⛵

@romaricpascal
Copy link
Member

Whoops, little linting issue before we can merge it seems. Strange that Husky didn't pick it up, but maybe it's because the commits were moved with rebases 🤔

We always want JavaScript and Stylesheets first
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4523 December 1, 2023 17:25 Inactive
@colinrotherham
Copy link
Contributor Author

Whoops, little linting issue before we can merge it seems. Strange that Husky didn't pick it up, but maybe it's because the commits were moved with rebases 🤔

Yeah all sorted, the basename import was in another commit

The ones in this PR were cherry picked so Husky doesn't kick in

@colinrotherham
Copy link
Contributor Author

@romaricpascal See what Brett means about the 4x comments on every PR, like this one?

@colinrotherham colinrotherham merged commit df4f316 into main Dec 1, 2023
45 checks passed
@colinrotherham colinrotherham deleted the ordered-diff branch December 1, 2023 17:31
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.

None yet

3 participants