Skip to content

Conversation

@jonahtanjz
Copy link
Contributor

@jonahtanjz jonahtanjz commented Jun 27, 2021

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Overview of changes:
Resolves #1601.

By using ::before to create spacing before a panel, this blocks certain elements before the panel which will hinder interactive elements (buttons not clickable etc.). The spacing is needed so that panels do not get blocked by the fixed-header when using anchor navigation.

Replaced ::before with scroll-margin-top which similarly creates spacing before the panels when using anchor navigation and also do not block other elements.

Anything you'd like to highlight / discuss:
If there is a better workaround.

Testing instructions:

  1. Create panel with header
<panel header="# Test">
</panel>
  1. Click on panel link in page-nav
  2. Panel should appear below the fixed-header
  3. Can refer to User Guide: Unable to open details panel on mobile #1601 issue, visit nav bar section in UG, details panel should be clickable now

Proposed commit message: (wrap lines at 72 characters)
Fix blocking ::before pseudo element in panel

Using ::before in panels may block interactive elements before
the panels. The spacing created using ::before is needed so that
panels do not get blocked by the fixed header when using anchor
navigation.

Let's replace ::before with scroll-margin-top which similarly creates
spacing before the panels when using anchor navigation and also do
not block other elements.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No blatantly unrelated changes
  • Pinged someone for a review!

@jonahtanjz jonahtanjz changed the title Replace panel before element with scroll-margin-top Replace panel ::before element with scroll-margin-top Jun 27, 2021
@ang-zeyu ang-zeyu merged commit a00655c into MarkBind:master Jun 27, 2021
@ang-zeyu ang-zeyu added this to the 3.0.3 milestone Jun 27, 2021
@ang-zeyu
Copy link
Contributor

Thanks @jonahtanjz! Didn't know this property existed (was going to suggest width: 1px or avoid assigning the ids to the <div> version of panels).

We can probably explore this as an alternative to the dummy <span>s inserted for headings with ids as well (or even universally applying this to everything under a fixed-header-padding container with an id).

@ang-zeyu
Copy link
Contributor

edit: might have jumped the gun here a little after getting excited with this property 😅 --

wonder if we should revert this for now and keep in view, seems like support for it on recent ios releases isn't too good yet (
https://caniuse.com/?search=scroll-margin-top) Scroll to fragment which is what we're depending on here especially is also bugged apparently, even with its in house scroll-snap-margin-top property.

@jonahtanjz
Copy link
Contributor Author

jonahtanjz commented Jun 27, 2021

wonder if we should revert this for now and keep in view, seems like support for it on recent ios releases isn't too good yet (
https://caniuse.com/?search=scroll-margin-top) Scroll to fragment which is what we're depending on here especially is also bugged apparently, even with its in house scroll-snap-margin-top property.

Hmm alright didn't know it was bugged on ios. Will raise another PR to temporary fix this using one of the suggestions. Yup we can probably wait till it is fully supported then replace the existing approach to fixed-header-padding.

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.

User Guide: Unable to open details panel on mobile

2 participants