Skip to content

Workaround missing navbar @768px for dspace and custom templates#1861

Merged
tdonohue merged 1 commit intoDSpace:mainfrom
the-library-code:PRs/FixedHeader768px
Sep 29, 2022
Merged

Workaround missing navbar @768px for dspace and custom templates#1861
tdonohue merged 1 commit intoDSpace:mainfrom
the-library-code:PRs/FixedHeader768px

Conversation

@hpwBLN
Copy link
Copy Markdown

@hpwBLN hpwBLN commented Sep 27, 2022

Attribution: Donated by The Library Code

Description

Workaround: missing navbar @768px in dspace and custom templates

Instructions for Reviewers

If the screen is 768px wide, the navbar is not showing.

The treatment of the bootstrap breakpoints in DSpace is inconsistent.

md is defined as 768px <= md < 992px.

The statement

@media screen and (max-width: map-get($grid-breakpoints, md)) {}

which is in some instances used at the moment for referencing break intervals BELOW md, includes the lower md border which should be excluded.

The sass mixin
@include media-breakpoint-down($name, $grid-breakpoints) {}

or function
breakpoint-max($name, $grid-breakpoints)

which are meant to be used in this case, for some unknown reason don't seem to work atm, so the usage of

@media screen and (max-width: map-get($grid-breakpoints, md)-0.02) {}

is a solution for the moment, but this is only a workaround.
The usage of "-0.02" instead of e.g. "-0.01" is a workaround for a rounding error in Safari.

See also:
https://getbootstrap.com/docs/4.6/layout/overview/

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

The treatment of the breakpoints is inconsistent. md is defined as
768px<=md<992px.
The statement
@media screen and (max-width: map-get($grid-breakpoints, md)) {}
,which is in some instances used at the moment for referencing break intervals BELOW md includes the lower md border which should be excluded.
The sass mixin
@include media-breakpoint-down($name) {}
or function
breakpoint-max($name)
for some unknown reason don't work, so this is a workaround.
@hpwBLN hpwBLN closed this Sep 27, 2022
@hpwBLN hpwBLN reopened this Sep 27, 2022
@tdonohue tdonohue added bug usability themes 1 APPROVAL pull request only requires a single approval to merge labels Sep 27, 2022
@tdonohue
Copy link
Copy Markdown
Member

@artlowel : Would you or someone on your team have a chance to glance at this small PR this week? It might be worth considering for 7.4, if it's non-controversial

Copy link
Copy Markdown
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @hpwBLN!

@hpwBLN
Copy link
Copy Markdown
Author

hpwBLN commented Sep 29, 2022

@artlowel, my pleasure. Thank you for reviewing.

@tdonohue tdonohue added this to the 7.4 milestone Sep 29, 2022
@tdonohue tdonohue merged commit 7b42446 into DSpace:main Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge bug themes usability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants