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

ER-864 card progress bar #966

Merged
merged 2 commits into from
Nov 16, 2023
Merged

ER-864 card progress bar #966

merged 2 commits into from
Nov 16, 2023

Conversation

peterdavidhamilton
Copy link
Contributor

@peterdavidhamilton peterdavidhamilton commented Nov 15, 2023

ER-864

Screenshot 2023-11-15 at 15 32 35

New resources using a %{num} variable:

  • my_learning.progress.percentage
  • my_learning.progress.viewed
  • my_learning.progress.remaining

Copy link

viezly bot commented Nov 15, 2023

Changes preview:

Legend:

👀 Review pull request on Viezly

@jack-coggin
Copy link
Contributor

Looks like the maintenance page redirect is still taking place in the CI tests, causing some failures

Copy link

@peterdavidhamilton
Copy link
Contributor Author

Looks like the maintenance page redirect is still taking place in the CI tests, causing some failures

It would help if I merged/rebased my own change 😑 @jack-coggin

- Add coverage and locales
- Adding write permission for deploy URL comment
- Add missing preference to seeded user
@instantrick
Copy link

@peterdavidhamilton Have you considered replacing the class "description" with "govuk-!-font-size-14" as this will be responsive and we won't need the extra declaration

@peterdavidhamilton
Copy link
Contributor Author

peterdavidhamilton commented Nov 16, 2023

@peterdavidhamilton Have you considered replacing the class "description" with "govuk-!-font-size-14" as this will be responsive and we won't need the extra declaration

I certainly would have before I refactored to see it was the only attribute required; but I am a fan of styling components in the CSS more than templates usually. Is there a visible difference in behaviour between the two or is it down to taste?

Not down to taste as such and there is no visible difference. It is just that we are creating a new class and bit of code to manage when there is no other styling on that element other than size and the controller for that will already exist in the core gov css.

@peterdavidhamilton
Copy link
Contributor Author

Not down to taste as such and there is no visible difference. It is just that we are creating a new class and bit of code to manage when there is no other styling on that element other than size and the controller for that will already exist in the core gov css.

If pressed I'd actually move .govuk-heading-s out of the template and define that in the style partial. Structure in one, appearance in the other.
WW @jack-coggin D?

@jack-coggin
Copy link
Contributor

Not down to taste as such and there is no visible difference. It is just that we are creating a new class and bit of code to manage when there is no other styling on that element other than size and the controller for that will already exist in the core gov css.

If pressed I'd actually move .govuk-heading-s out of the template and define that in the style partial. Structure in one, appearance in the other. WW @jack-coggin D?

My preference would be to do as much of the styling as possible in the css rather than the template - mainly for readability/maintainability

@peterdavidhamilton
Copy link
Contributor Author

@instantrick something that breaks the lovely clean lines of SLIM is the ! in the class names which forces you to employ class=foo-!-bar which kind of irks me. @jack-coggin I bet you're not surprised 😄

@peterdavidhamilton peterdavidhamilton added this pull request to the merge queue Nov 16, 2023
Merged via the queue into main with commit 4371b56 Nov 16, 2023
3 checks passed
@peterdavidhamilton peterdavidhamilton deleted the ER-864-card-progress-bar branch November 16, 2023 10:51
@instantrick
Copy link

@instantrick something that breaks the lovely clean lines of SLIM is the ! in the class names which forces you to employ class=foo-!-bar which kind of irks me. @jack-coggin I bet you're not surprised 😄

Yep that is fair enough. I the class with -s won't push it down. For now it's fine. We're talking about saving bytes ultimately so I am happy to go with the fix of just adding the missing px in the css

Copy link

@instantrick instantrick left a comment

Choose a reason for hiding this comment

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

All looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Changes to assets detected pipeline Github workflow changes review Review app deployed to Azure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants