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

Make metrics page font size consistent #1738

Merged
merged 6 commits into from
Feb 2, 2023
Merged

Make metrics page font size consistent #1738

merged 6 commits into from
Feb 2, 2023

Conversation

kushalnl7
Copy link
Contributor

Regarding the issue mentioned in #1695 which focuses on inconsistency in fonts on different pages.

Description

As mentioned in #1695 using the two images of different pages, metrics page vs normal page, the former had a navbar menu with inconsistent font size than the others. After looking at the code, I could understand that the default value for navbar menu font is given to be "1rem". Hence, it takes the font-size from the root element.

Root element for normal page -

Screenshot from 2023-01-30 20-47-06

Root element for metrics page -

Screenshot from 2023-01-30 20-46-49

Due to this difference, there was a inconsistency in the font-size of navbar menu options. Hence, to resolve this, I added a new class to the navbar and defined a font-size for menu options in a css file.

Navbar menu options font after changes -

Screenshot from 2023-01-30 20-51-02

This makes the font-size consistent for all pages.

Motivation and Context

This solution is in regards to the issue - #1695

How Has This Been Tested?

The given additions resolve the issue as shown in above images and it does not affect other areas of code.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

@damianhxy damianhxy linked an issue Jan 30, 2023 that may be closed by this pull request
Copy link
Member

@damianhxy damianhxy left a comment

Choose a reason for hiding this comment

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

Hi @kushalnl7, thank you for this PR.

I took a closer look at the issue, and I realized the main issue was that metrics.scss does an @import "semantic-ui" which imports semantic-ui/globals/all. This unintentionally sets the font-size to 14px.

Screenshot 2023-01-30 at 14 26 16

In contrast, the rest of the website that does not have this import statement will default to materialize's 15px (application.scss imports materialize, which imports materialize/components/typography).

Screenshot 2023-01-30 at 14 27 45

My recommendation

To target the root cause, could you instead set html's font-size to 15px inside assessts/stylesheets/metrics.scss?

Screenshot 2023-01-30 at 14 29 44

Let me know if you agree!

@kushalnl7
Copy link
Contributor Author

That's a great observation @damianhxy. I completely missed that. Thanks for pointing it out.

But, the given solution would create more inconsistency as it sets up 15px size for the root element irrespective of the window-size. While the other pages would jump between {15px, 14.5px, 14px} depending on the window-size.

Normal pages for lesser window size vs the 15px size of metrics page -

image

Hence, the solution for this could be -

  1. To go for the current solution to declare a font size in navbar component itself, which would help us maintain the consistency for all the pages.
  2. To add the css media queries to the metrics.scss file, similar to the other pages. This would go something like this -

image

My opinion - I believe that the first method would work better in our situation because the alternative could lead to a recurrence of the font inconsistency problem as more new pages are added. The first option would give the developer the freedom to utilise various stylesheets without thinking about this problem. Let me know your views on this.

@damianhxy
Copy link
Member

damianhxy commented Jan 31, 2023

Hi @kushalnl7,

My thoughts on the first method is that declaring a font size on the navbar component is not addressing the root cause of the problem, and can lead to further inconsistencies down the road if future elements with font-size: 1rem are created on the metrics page. It also obscures the intent to inherit the font size value from the root element.

For that reason, I prefer the second option, as we will be resetting the "environment variables" of the page by undoing unwanted changes by semantic-ui. While it is true that such logic might have to be duplicated when utilizing various stylesheets, I feel that the onus should be on a developer to ensure that their styles are scoped appropriately and do not overwrite existing styles. Unfortunately, there does not appear to be a way to customize our version of semantic-ui.

Thus, my recommendation would be to add the media queries to the metrics.scss file, and perhaps add a comment explaining the rationale for doing so.

In fact, we could do it cleaner by adding a @import "materialize/components/typography" to the start of the metrics file.

Let me know your thoughts on this!

@kushalnl7
Copy link
Contributor Author

Hi @damianhxy , yes I think you are right about the inconsistencies that the 1st solution could create in future. Hence, I would also prefer for the second solution, considering that the future developers would keep this thing on their priority to maintain this consistency. While I am doubtful of directly importing @import "materialize/components/typography" on top of the "semantic-ui" import as it might bring more inconsistencies to the page due to some conflicts in styles. What do you think about this?

@damianhxy
Copy link
Member

@kushalnl7 That is true, importing materialize again is a rather heavy-handed approach. In that case, it would be fine to duplicate media queries to set the font-size, and leave a comment such as /* reset changes by semantic-ui */

@kushalnl7
Copy link
Contributor Author

Hi @damianhxy, I have pushed the changes as per our discussion. Please review them. Thanks!

@kushalnl7
Copy link
Contributor Author

Ohh, sorry for that. Resolved it now!

Copy link
Member

@damianhxy damianhxy left a comment

Choose a reason for hiding this comment

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

LGTM, navbar text correctly sized on metrics page and responsive

@kushalnl7
Copy link
Contributor Author

Hi @damianhxy, I have committed the update. Thanks for pointing it out!

@damianhxy damianhxy changed the title Resolved The Font Consistency Issue Of Metrics Navbar Make metrics page font size consistent Feb 2, 2023
@damianhxy damianhxy merged commit 2d585ca into autolab:master Feb 2, 2023
@kushalnl7
Copy link
Contributor Author

Thanks @damianhxy.

@kushalnl7 kushalnl7 deleted the navbar_font_consistency branch February 2, 2023 19:17
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.

Metrics page navbar has smaller font
2 participants