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

[#11724] Admin page header tabs do not fit header at certain screen sizes #11799

Merged
merged 7 commits into from
May 21, 2022

Conversation

parthnatu
Copy link
Contributor

Fixes #11724

Added "Utilities" dropdown to group admin tools. Open to a different name if appropriate.

@parthnatu
Copy link
Contributor Author

image

@zhaojj2209
Copy link
Contributor

Thank you for contributing to TEAMMATES!

While this does solve the visual bug, grouping every admin tab under a single "Utilities" dropdown may not be ideal UX-wise. There are two possible ways to expand on this solution:

  1. Grouping the less commonly used tabs (e.g. Timezone Listing and Usage Statistics under a "More" dropdown)
  2. Grouping similar tabs under a suitably named dropdown (e.g. Logs and Usage Statistics under a "Stats" dropdown)

@damithc would like your input on this, which presentation would you prefer?

@zhaojj2209 zhaojj2209 added s.Ongoing The PR is being worked on by the author(s) s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels May 18, 2022
@damithc
Copy link
Contributor

damithc commented May 18, 2022

  1. Grouping the less commonly used tabs (e.g. Timezone Listing and Usage Statistics under a "More" dropdown)
  2. Grouping similar tabs under a suitably named dropdown (e.g. Logs and Usage Statistics under a "Stats" dropdown)

@damithc would like your input on this, which presentation would you prefer?

I prefer 1.

@parthnatu
Copy link
Contributor Author

parthnatu commented May 18, 2022

  1. Grouping the less commonly used tabs (e.g. Timezone Listing and Usage Statistics under a "More" dropdown)
  2. Grouping similar tabs under a suitably named dropdown (e.g. Logs and Usage Statistics under a "Stats" dropdown)

@damithc would like your input on this, which presentation would you prefer?

I prefer 1.

Thanks, @zhaojj2209 and @damithc :)
Got it. So what items would be a part of the "More" dropdown?

@zhaojj2209
Copy link
Contributor

Thanks, @zhaojj2209 and @damithc :) Got it. So what items would be a part of the "More" dropdown?

Grouping the "Timezone Listing" and "Usage Statistics" tabs together should be sufficient.

@parthnatu
Copy link
Contributor Author

I tried doing that but the glitch still occurs when we hit 992px width.

image

@damithc
Copy link
Contributor

damithc commented May 18, 2022

Also note that the problem occurs only during a certain width range. As I reduce the browser width incrementally, this is what happens:

No problem, all menu is shown in one row
Problem appears
No problem, all menu is shown in one row (this is the odd part)
Responsiveness mechanism kick in -> menu transforms into a hamburger format

So, the cause may not be the number of items in the menu but something else.

@parthnatu
Copy link
Contributor Author

parthnatu commented May 18, 2022

Note: I reverted to the original navbar for these findings
Yes. I observed that as well. These were my findings:

  • The problem appears twice on my end. Once when we hit 1097px when the app_admin dropdown overlaps with the other items in the navbar:

image

  • The problem gets fixed at 1024px where the max-width trigger kicks in:

image

  • This continues till 997px where the app_admin dropdown overlaps with the items again and the issue reappears:

image

  • The issue persists till 991px where the hamburger menu kicks in:

image

So the issue seems to be the app_admin dropdown overlapping.

@samuelfangjw
Copy link
Member

samuelfangjw commented May 18, 2022

Also note that the problem occurs only during a certain width range. As I reduce the browser width incrementally, this is what happens:

No problem, all menu is shown in one row Problem appears No problem, all menu is shown in one row (this is the odd part) Responsiveness mechanism kick in -> menu transforms into a hamburger format

So, the cause may not be the number of items in the menu but something else.

This is caused because the amount of space allocated to the nav items changes at certain breakpoints (in fact, this issue occurs for some of the other non-admin pages as well). So two factors at play here, navbar items taking up too much space + space allocated for navbar items might not be enough at certain breakpoints.

While this could be solved by tweaking the spacing at each breakpoint, in my opinion this should not be the only solution (could be part of the solution) because:

  1. When we inevitably add more pages, this issue will occur again.
  2. We might have to use non-standard breakpoints (e.g. something in the middle of small and medium breakpoints defined by bootstrap). This might lead to UI inconsistencies (extreme example: the navbar is in the mobile state (with hamburger menu) but the other UI elements are still in 'desktop' state) and elements resizing at different breakpoints when the window is resized, which while not a big issue is a bit of an eye sore.

In my opinion, the primary way of solving this should be to reduce the number of navbar items in a row by grouping related nav items together (which is what is being done here). In addition to this, we could tweak the spacing slightly if necessary (across the different pages as well to maintain consistency).

@zhaojj2209
Copy link
Contributor

I tried doing that but the glitch still occurs when we hit 992px width.

After some digging, I think this may be due to an issue in with the way we styles are defined, see below snippet from page.component.scss:

@media (max-width: 992px) {
  .navbar-nav li {
    margin: auto 20px;
  }
}

The bootstrap breakpoints indicate the lower bound of the size range (e.g. lg means sizes >=992px), but the custom styles use the breakpoint as an upper bound (e.g. the above style is applied to sizes <=992px). This causes styles to be wrongly applied at the exact breakpoint widths (in this case, at 992px the navbar is expanded, but the above style is also applied even though it should only be applied when the navbar is collapsed).

@parthnatu
Copy link
Contributor Author

I tried doing that but the glitch still occurs when we hit 992px width.

After some digging, I think this may be due to an issue in with the way we styles are defined, see below snippet from page.component.scss:

@media (max-width: 992px) {
  .navbar-nav li {
    margin: auto 20px;
  }
}

The bootstrap breakpoints indicate the lower bound of the size range (e.g. lg means sizes >=992px), but the custom styles use the breakpoint as an upper bound (e.g. the above style is applied to sizes <=992px). This causes styles to be wrongly applied at the exact breakpoint widths (in this case, at 992px the navbar is expanded, but the above style is also applied even though it should only be applied when the navbar is collapsed).

Thanks for the suggestion! I changed the value to 15px and it works.

@zhaojj2209
Copy link
Contributor

Thanks for the suggestion! I changed the value to 15px and it works.

The tabs are indeed on the same row now, but this fix doesn't solve the underlying issue, which is that the extra margin is added at 992px even though it's not supposed to be.

Rather than change the margin, I would suggest changing max-width from 992px to 991px instead.

@parthnatu
Copy link
Contributor Author

Thanks for the suggestion! I changed the value to 15px and it works.

The tabs are indeed on the same row now, but this fix doesn't solve the underlying issue, which is that the extra margin is added at 992px even though it's not supposed to be.

Rather than change the margin, I would suggest changing max-width from 992px to 991px instead.

Ah yes. Makes sense. Made that change.

Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

Almost there, just one last nit from me

src/web/app/pages-admin/admin-page.component.ts Outdated Show resolved Hide resolved
@parthnatu
Copy link
Contributor Author

Almost there, just one last nit from me

Fixed it now.

Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

LGTM!

@zhaojj2209 zhaojj2209 added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels May 19, 2022
@parthnatu
Copy link
Contributor Author

LGTM!

Great 😊

Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution!

@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Bug Bug/defect report and removed s.FinalReview The PR is ready for final review labels May 21, 2022
@wkurniawan07 wkurniawan07 merged commit eadffc2 into TEAMMATES:master May 21, 2022
@wkurniawan07 wkurniawan07 added this to the V8.16.0 milestone May 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin page header tabs do not fit header at certain screen sizes
5 participants