Limit height of page content so that app bar and nav bar stays visible when scrolling #1121

kennethnym · 2022-07-07T13:05:53Z

Description

This PR limits the height of PageContainer so that the app bar and the footer is still visible when the page is scrolling.

Demo

This clip shows the new scroll behavior:

SciGateway.and.10.more.pages.-.Work.-.Microsoft.Edge.2022-07-07.15-39-55.mp4

Testing instructions

Review code
Check Actions build
Review changes to test coverage

Agile board tracking

connect to #996

Closes #996

codecov · 2022-07-07T13:07:37Z

Codecov Report

Merging #1121 (1dc3be6) into develop (ea067ff) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1121      +/-   ##
===========================================
- Coverage    97.66%   97.66%   -0.01%     
===========================================
  Files           42       42              
  Lines         1586     1585       -1     
  Branches       433      432       -1     
===========================================
- Hits          1549     1548       -1     
  Misses          34       34              
  Partials         3        3

Impacted Files	Coverage Δ
src/routing/routing.component.tsx	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea067ff...1dc3be6. Read the comment docs.

…ity-pages-#996 # Conflicts: # src/helpPage/helpPage.component.tsx

…ity-pages-#996

sam-glendenning

UI-wise it looks good but as Louise said we still have an extra vertical scroll bar present in table view. This mostly affects Dataview from what I can see but I had a look elsewhere and saw maybe a couple of other areas that could maybe be improved too.

For example, on the admin download table, we have an extra scroll bar that we probably don't need and this is because the table element is surrounded by what looks to be a Paper component. I removed it and it got rid of the problem but also caused the whole section to be moved too far up and left, like so:

Have a play around with it and see if you can get it lining up nicely, I think it just needs some padding somewhere.

Similarly, I noticed the same problem on the downloadTab component in DataGateway Download which I think might be caused by this? So if you fancy having a go at that one too, that would be cool, would just require a PR on DataGateway repo.

src/routing/routing.component.test.tsx

kennethnym · 2022-07-29T15:48:58Z

Thank you for the review! I'll look into the scroll bar issue.

sam-glendenning · 2022-08-01T12:43:27Z

src/routing/routing.component.test.tsx

+ <ThemeProvider theme={theme}>
+ <Routing />
+ </ThemeProvider>


Sorry, one last change! Adding the ThemeProvider to the mounted wrapper has resulted in some massive additions to the snapshots that we just don't need. If you take it out from all the mounted wrappers in this file, you should get some more manageable snapshots. Once it's done, compare the new snapshots to what is currently on develop branch and as long as they look fairly similar in size, they're probably good to go!

Oh I just realised. The ThemeProvider was what I initially told you to change. And now I realise that it's not even needed. Really sorry! I owe you one for this 😅

No worries! I didn't know it wasn't needed. I thought the Routing component would need it but turns out it doesn't. I don't think it should be removed though as we will have to rewrite it in @testing-library in the future which will actually require ThemeProvider since it renders the entire component out, and all the styling obviously requires the theme object to be in context.

Personally, if we remove it, we can more easily test the snapshot is correct during the migration to testing-library, plus just leaving the import by itself will make the linter complain. But that's just my opinion

sam-glendenning · 2022-08-01T13:18:40Z

Also I think something might have gone wrong with the table in dataview... the bottom has been cut off

kennethnym · 2022-08-01T13:58:16Z

I think it is due to a slight miscalculation of height in datagateway-dataview. I played around with the number and I found that replacing calc(100vh - 180px - 36px - 1px - ${linearProgressHeight} - ${breadcrumbHeight}) with calc(100vh - 152px -36px - 1px - ${linearProgressHeight} - ${breadcrumbHeight}) makes the container fit perfectly, although I don't really know why it is specifically 152px...

sam-glendenning · 2022-08-01T14:00:07Z

I thought it could be something like that. Try the solution on a bunch of pages before submitting it, just to make sure it doesn't accidentally break something else!

kennethnym · 2022-08-01T14:07:50Z

The calculation is only for datagateway-dataview's table (specifically this line), so it shouldn't affect other pages, but I viewed other pages normally and in the responsive tool and couldn't produce any regression.

sam-glendenning

Tested this with ral-facilities/datagateway#1378 and it works well!

We do now have a snapshot that's bigger than it's worth but as long as we put a pin in it to remember to reduce it when we refactor tests, it can stay for now

kennethnym · 2022-08-02T13:41:40Z

It should become smaller as we slowly migrate to @testing-library because instead of snapshotting the theme object, it snapshots the actual HTML output as if it were in a browser.

Limit height of help & a11y page so nav is visible

efb49ef

kennethnym added the enhancement New feature or request label Jul 7, 2022

kennethnym linked an issue Jul 7, 2022 that may be closed by this pull request

Scrolling for Help and Accessibility statement pages #996

Closed

1 task

Limit height of PageContainer

b1cc7b4

kennethnym changed the title ~~Limit height of help & a11y page so that the navigation is still visible~~ Limit height of page content so that app bar and nav bar stays visible when scrolling Jul 7, 2022

kennethnym added 4 commits July 8, 2022 15:41

Merge branch 'develop' into feature/scrolling-for-help-and-accessibil…

bb5057a

…ity-pages-#996 # Conflicts: # src/helpPage/helpPage.component.tsx

Update Routing style

721cc0b

Merge branch 'develop' into feature/scrolling-for-help-and-accessibil…

877c909

…ity-pages-#996

Update snapshots to reflect scrolling style change

3aa1d69

louise-davies requested review from louise-davies and sam-glendenning and removed request for louise-davies July 28, 2022 13:13

sam-glendenning suggested changes Jul 29, 2022

View reviewed changes

src/routing/routing.component.test.tsx Outdated Show resolved Hide resolved

kennethnym added 3 commits August 1, 2022 11:52

Adjust height calculation of scroll container

a81fe87

Import ThemeProvider from mui instead of emotion

2111aa4

Update snapshot because of ThemeProvider change

1dc3be6

sam-glendenning reviewed Aug 1, 2022

View reviewed changes

kennethnym mentioned this pull request Aug 1, 2022

Adjust height calculations in various components ral-facilities/datagateway#1378

Merged

3 tasks

sam-glendenning approved these changes Aug 2, 2022

View reviewed changes

kennethnym merged commit 30d01c6 into develop Aug 2, 2022

joelvdavies mentioned this pull request Jul 8, 2024

Fix help page top-of-page-icon link on help page #1400

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit height of page content so that app bar and nav bar stays visible when scrolling #1121

Limit height of page content so that app bar and nav bar stays visible when scrolling #1121

kennethnym commented Jul 7, 2022 •

edited

Loading

codecov bot commented Jul 7, 2022 •

edited

Loading

sam-glendenning left a comment

kennethnym commented Jul 29, 2022

sam-glendenning Aug 1, 2022

sam-glendenning Aug 1, 2022

kennethnym Aug 1, 2022

sam-glendenning Aug 1, 2022

sam-glendenning commented Aug 1, 2022

kennethnym commented Aug 1, 2022

sam-glendenning commented Aug 1, 2022

kennethnym commented Aug 1, 2022 •

edited

Loading

sam-glendenning left a comment

kennethnym commented Aug 2, 2022

Limit height of page content so that app bar and nav bar stays visible when scrolling #1121

Limit height of page content so that app bar and nav bar stays visible when scrolling #1121

Conversation

kennethnym commented Jul 7, 2022 • edited Loading

Description

Demo

Testing instructions

Agile board tracking

codecov bot commented Jul 7, 2022 • edited Loading

Codecov Report

sam-glendenning left a comment

Choose a reason for hiding this comment

kennethnym commented Jul 29, 2022

sam-glendenning Aug 1, 2022

Choose a reason for hiding this comment

sam-glendenning Aug 1, 2022

Choose a reason for hiding this comment

kennethnym Aug 1, 2022

Choose a reason for hiding this comment

sam-glendenning Aug 1, 2022

Choose a reason for hiding this comment

sam-glendenning commented Aug 1, 2022

kennethnym commented Aug 1, 2022

sam-glendenning commented Aug 1, 2022

kennethnym commented Aug 1, 2022 • edited Loading

sam-glendenning left a comment

Choose a reason for hiding this comment

kennethnym commented Aug 2, 2022

kennethnym commented Jul 7, 2022 •

edited

Loading

codecov bot commented Jul 7, 2022 •

edited

Loading

kennethnym commented Aug 1, 2022 •

edited

Loading