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

Adjust chart component options #4342

Merged
merged 3 commits into from
Oct 25, 2024
Merged

Adjust chart component options #4342

merged 3 commits into from
Oct 25, 2024

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Oct 25, 2024

What

Change the chart component. Specifically:

  • remove the requirement for a link from the minimal mode
  • adds an option to pad the text and elements around the graph

Why

For a minimal chart we thought that it should link to a page with more data, but this has proved complicated in an actual page, and now I think it's better handled in an application rather than dictated by the component.

Padding is needed for when the chart is inside an element with a border, as these elements touch the outer edge.

Visual Changes

No change to the minimal version, except that it no longer has a link.

Padded version shown below with the normal version for comparison (it's a bit subtle, look at the left edge).

Screenshot 2024-10-25 at 15 12 51

Trello card: https://trello.com/c/VKrLuGvg

@andysellick andysellick changed the title Adjust chart options Adjust chart component options Oct 25, 2024
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4342 October 25, 2024 14:14 Inactive
Copy link
Contributor

@AshGDS AshGDS left a comment

Choose a reason for hiding this comment

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

Looks good, does it need a changelog entry?

I've also added a small comment about one of the lines in the variant description.

Also, if you rebase with main the Percy diff should be more accurate.

andysellick added a commit to alphagov/frontend that referenced this pull request Oct 25, 2024
- bring the text around charts in a bit, as they're currently colliding with the edge of their containing element when shown inside a card block
- this option at time of writing isn't available, will be added in alphagov/govuk_publishing_components#4342
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4342 October 25, 2024 15:21 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4342 October 25, 2024 15:23 Inactive
- whilst this seemed like a good idea at the time, it presents some issues, notably that the chart has nothing that indicates it is a link and can be clicked on
- better to remove the link entirely and provide better guidance on how minimal mode should be used i.e. manage linking to or showing more data at an application level, rather than the component level
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4342 October 25, 2024 15:24 Inactive
Copy link
Contributor

@AshGDS AshGDS left a comment

Choose a reason for hiding this comment

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

Thanks 👍

leenagupte pushed a commit to alphagov/frontend that referenced this pull request Oct 25, 2024
- bring the text around charts in a bit, as they're currently colliding with the edge of their containing element when shown inside a card block
- this option at time of writing isn't available, will be added in alphagov/govuk_publishing_components#4342
leenagupte pushed a commit to alphagov/frontend that referenced this pull request Oct 25, 2024
- bring the text around charts in a bit, as they're currently colliding with the edge of their containing element when shown inside a card block
- this option at time of writing isn't available, will be added in alphagov/govuk_publishing_components#4342
leenagupte pushed a commit to alphagov/frontend that referenced this pull request Oct 25, 2024
- bring the text around charts in a bit, as they're currently colliding with the edge of their containing element when shown inside a card block
- this option at time of writing isn't available, will be added in alphagov/govuk_publishing_components#4342
@andysellick andysellick merged commit f420423 into main Oct 25, 2024
12 checks passed
@andysellick andysellick deleted the adjust-chart-options branch October 25, 2024 15:36
leenagupte pushed a commit to alphagov/frontend that referenced this pull request Oct 25, 2024
- bring the text around charts in a bit, as they're currently colliding with the edge of their containing element when shown inside a card block
- this option at time of writing isn't available, will be added in alphagov/govuk_publishing_components#4342
@andysellick andysellick mentioned this pull request Oct 25, 2024
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