Skip to content

Conversation

@jonahtanjz
Copy link
Contributor

@jonahtanjz jonahtanjz commented Jun 28, 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:
Follow-up to #1603 (comment). The support of scroll-margin-top on ios is still limited thus require a different approach to handle anchor navigation for panels.

Replace existing panel id with span.anchor (similar to how normal headers navigation works). This prevents the panels from getting blocked by the fixed-header bar as well as resolves #1601.

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

Apparently changing the width of ::before does not fix #1601 as the entire panel block still occupies the vertical space of ::before as well.
image

The container which holds ::before still has the width of the panel
image

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)
Implement span.anchor in panel to replace id

The support of scroll-margin-top on ios is still limited hence
would require a different approach to handle anchor navigation
for panels while keeping elements before panels from getting
blocked.

Let's introduce span.anchor into panel, similar to how the headers
navigation works, to support anchor navigation as well as to prevent
previous elements from getting blocked.


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!

import panelBase from './PanelBase';

export default {
inheritAttrs: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm what's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vue passes non-prop attributes such as id to the root node of a component. In this case, we do not want the main panel container, div or span, to have the id as this interferes with anchor navigation.
The inheriAttrs: false is needed to prevent the root node of minimalPanel and nestedPanel from inheriting the id attribute.

https://v3.vuejs.org/guide/component-attrs.html#disabling-attribute-inheritance

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying! Hmm how about changing the property name under assignPanelId (then using it as a prop)?

So there's no risk of side effects from disabling attribute inheritance entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup that would work as well. Will update it.

@jonahtanjz
Copy link
Contributor Author

@ang-zeyu Updated the PR to pass in panelId as prop.

@jonahtanjz jonahtanjz force-pushed the Fix-blocking-panel-element branch from 24c81ba to 35357dd Compare July 3, 2021 03:52
@ang-zeyu
Copy link
Contributor

ang-zeyu commented Jul 5, 2021

Lgtm 👍, just this portion that would have to be edited also since we aren't using ::before anymore.

(handles the auto scroll back to top)

// To enable behaviour of auto window scrolling during panel collapse
if (this.$el.getBoundingClientRect().top < 0) {
  jQuery('html').animate({
    scrollTop: window.scrollY + this.$el.getBoundingClientRect().top - 3,
  }, 500, 'swing');
}

@jonahtanjz jonahtanjz force-pushed the Fix-blocking-panel-element branch from 35357dd to 69ef766 Compare July 5, 2021 13:26
@jonahtanjz
Copy link
Contributor Author

@ang-zeyu Thanks for the catch. Have updated the PR :)

requestAnimationFrame(() => {
// To enable behaviour of auto window scrolling during panel collapse
if (this.$el.getBoundingClientRect().top < 0) {
const headerHeight = jQuery('header[fixed]').height() || 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

a really minor nit for keeping everything under the panel implementation -- would this work?

const el = $refs.anchor || this.$el;
el .getBoundingClientRect......

Copy link
Contributor Author

@jonahtanjz jonahtanjz Jul 8, 2021

Choose a reason for hiding this comment

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

Hmm added ref="anchor" to the spans and doesn't seem to be working... The panel header still gets hidden under the navbar after collapsing.

Copy link
Contributor

Choose a reason for hiding this comment

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

try adding display: inline-block to the scoped anchor class here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slightly better but half of it is still under the navbar... When lower navbar is present, it gets hidden again 😅

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the sibling after span.anchor is interfering 🤣... let's leave it for now and revisit later

Copy link
Contributor

@ang-zeyu ang-zeyu left a comment

Choose a reason for hiding this comment

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

Lgtm 👍

@ang-zeyu ang-zeyu added this to the v3.0.5 milestone Jul 8, 2021
@ang-zeyu ang-zeyu merged commit a07107f into MarkBind:master Jul 8, 2021
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