-
Notifications
You must be signed in to change notification settings - Fork 7
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
Change all title components to heading components #3647
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes seem good.
Next time can you link to the URLs affected rather than the files as it would make reviewing the changes easier. Also if there are visual differences screenshots would be useful to check them.
@@ -7,7 +7,6 @@ | |||
heading_level: 1, | |||
font_size: "xl", | |||
margin_bottom: 4, | |||
inverse: inverse, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, just noticed this - what was this change for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't replicate a view where there is an inverse header, so I thought it was an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd be more comfortable with this change supported by some more investigation. Since this is unrelated I suggest not deleting this line as part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I've reverted that and also the other two titles further down the page where I'd also removed the inverse option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks. I've spotted an unrelated problem with this app but I'd like to get your changes out first, I know you've just finished for today so apologies am going to merge this PR (if I can get the tests to pass).
909308d
to
79c6b42
Compare
Note that indentation of the main content changes on the index but this doesn't materially change anything.
79c6b42
to
208b079
Compare
What
Replace title components with headings. This PR covers the following views:
Why
We are attempting to retire the page title component in favour of the heading component. Trello
Before and After views:
Changes are minor and acceptable according to our criteria.