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

SF-3027 Fix inconsistent styles of page titles #2796

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kylebuss
Copy link
Collaborator

@kylebuss kylebuss commented Oct 10, 2024

This PR makes the styling of the pages title consistent across the app.


This change is Reviewable

@kylebuss kylebuss requested a review from josephmyers October 10, 2024 20:30
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.02%. Comparing base (6af6d6f) to head (3503086).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2796      +/-   ##
==========================================
- Coverage   79.02%   79.02%   -0.01%     
==========================================
  Files         530      530              
  Lines       30719    30719              
  Branches     5021     5016       -5     
==========================================
- Hits        24276    24275       -1     
+ Misses       5673     5661      -12     
- Partials      770      783      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kylebuss kylebuss added the will require testing PR should not be merged until testers confirm testing is complete label Oct 10, 2024
@RaymondLuong3 RaymondLuong3 self-assigned this Oct 10, 2024
Copy link
Collaborator

@RaymondLuong3 RaymondLuong3 left a comment

Choose a reason for hiding this comment

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

You should be able to apply styling using a material theme. I haven't tried this, but this article should give you some ideas. Theming Angular Material | Angular Material
I like the slim look of the characters for our headline labels. Sometimes it looks too large, such as on the checking overview page, but other than that, the consistency is a plus.

Reviewed 22 of 22 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @josephmyers and @kylebuss)


src/SIL.XForge.Scripture/ClientApp/src/material-styles.scss line 222 at r1 (raw file):

div {
  .title {

I would use a more descriptive class, like "headline-4" or just "headline"

Code quote:

  .title {

src/SIL.XForge.Scripture/ClientApp/src/material-styles.scss line 225 at r1 (raw file):

    @extend .mat-headline-4;
    margin-bottom: 0;
  }

You probably want this instead, what you have says select an element with the title class that is a child of a div element.

Code quote (i):

div {
  .title {
    @extend .mat-headline-4;
    margin-bottom: 0;
  }

Code snippet (ii):

div.title {
  @extend ...

@Nateowami
Copy link
Collaborator

After looking at this very briefly, I don't think I agree with either direction that's being discussed here. I suggest working on other things until we get a chance to talk and get on the same page.

@kylebuss kylebuss marked this pull request as draft October 11, 2024 12:55
Copy link
Collaborator Author

@kylebuss kylebuss left a comment

Choose a reason for hiding this comment

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

@Nateowami and @RaymondLuong3, any update on where we are at on this one? Should we schedule a time to meet?

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @josephmyers)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
will require testing PR should not be merged until testers confirm testing is complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants