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

fix: update enterprise-app view height #1258

Merged
merged 8 commits into from
Jun 27, 2024
Merged

fix: update enterprise-app view height #1258

merged 8 commits into from
Jun 27, 2024

Conversation

brobro10000
Copy link
Member

@brobro10000 brobro10000 commented Jun 24, 2024

Prior to these changes, the view height of the main enterprise-app component would cause a UI bug where if the content was less then the height of the sidebar, the sidebar would 'clip' into the footer creating an unsightly experience for users.
We have updated the CSS to make the main contents min-height 100% of the view height with an offset to take into account the header/footer heights. The offset should avoid the user having to unnecessarily scroll down to see the footer.

Before:
Screenshot 2024-06-24 at 2 31 39 PM

After
Screenshot 2024-06-24 at 2 30 53 PM

For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 88.37209% with 5 lines in your changes missing coverage. Please review.

Project coverage is 85.38%. Comparing base (13acd81) to head (82415ee).

Files Patch % Lines
src/components/Footer/index.jsx 55.55% 4 Missing ⚠️
src/components/Header/index.jsx 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1258   +/-   ##
=======================================
  Coverage   85.38%   85.38%           
=======================================
  Files         541      545    +4     
  Lines       11933    11970   +37     
  Branches     2550     2562   +12     
=======================================
+ Hits        10189    10221   +32     
- Misses       1685     1690    +5     
  Partials       59       59           

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

const [globalState, dispatch] = useReducer(globalReducer, initialState);
const globalContext = useMemo(() => ({
...globalState,
// Offsets by an additional 1 rem to avoid rendering the scrollbar unnecessarily
Copy link
Member Author

Choose a reason for hiding this comment

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

Offset the viewheight by an additional 1rem to avoid the scrollbar rendering when the main content does not overflow.

Before 1rem offset:
https://github.com/openedx/frontend-app-admin-portal/assets/82611798/828bf096-1ae9-4df2-817b-21260ee40854

After 1rem offset:
https://github.com/openedx/frontend-app-admin-portal/assets/82611798/4d507e9d-3d19-4c8d-9aae-7f63d2bc954a

@brobro10000 brobro10000 force-pushed the hu/ent-9089 branch 2 times, most recently from e96a377 to b2c92d4 Compare June 26, 2024 16:03
@brobro10000 brobro10000 force-pushed the hu/ent-9089 branch 2 times, most recently from 5ae8b07 to 19b6f7b Compare June 26, 2024 17:16
@brobro10000 brobro10000 merged commit e74391e into master Jun 27, 2024
6 checks passed
@brobro10000 brobro10000 deleted the hu/ent-9089 branch June 27, 2024 19:14
adamstankiewicz added a commit that referenced this pull request Jul 8, 2024
adamstankiewicz added a commit that referenced this pull request Jul 8, 2024
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.

2 participants