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

Closes #151: Show headings in the sidebar #167

Closed
wants to merge 10 commits into from
Closed

Closes #151: Show headings in the sidebar #167

wants to merge 10 commits into from

Conversation

nblthree
Copy link
Member

@nblthree nblthree commented May 9, 2019

Summary
Closes #151
What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI/CSS related code, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

  • Show headings
  • Change the hash depending on the scroll position
  • Make sidebar sections's collapsible

@netlify
Copy link

netlify bot commented May 9, 2019

Deploy preview for saber ready!

Built with commit 4d2e31a

https://deploy-preview-167--saber.netlify.com

@krmax44
Copy link
Contributor

krmax44 commented May 9, 2019

Make sidebar section collapsible (???? sidebar section is already collapsible when the screen size is small)

I think it means adding a hamburger menu style button to allow collapsing it even on a desktop

@nblthree
Copy link
Member Author

nblthree commented May 9, 2019

I just think that there is no need to do that because it became collapsible when the window inner width is inferior to 768px

@egoist
Copy link
Collaborator

egoist commented May 9, 2019

屏幕快照 2019-05-09 下午9 54 35

I mean something like this.

@nblthree nblthree changed the title Closes #151: Show headings sidebar Closes #151: Show headings in the sidebar May 9, 2019
@nblthree
Copy link
Member Author

nblthree commented May 9, 2019

Seems that the deployment fell when importing ./file.vue that's what I understood from the last commit

@nblthree nblthree marked this pull request as ready for review May 10, 2019 10:21
@nblthree nblthree requested review from egoist and krmax44 May 10, 2019 17:37
Copy link
Contributor

@krmax44 krmax44 left a comment

Choose a reason for hiding this comment

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

Pretty good 👍 Only some code style aspects that could be improved.

website/src/components/Sidebar.vue Outdated Show resolved Hide resolved
website/src/components/Sidebar.vue Outdated Show resolved Hide resolved
website/src/components/Sidebar.vue Outdated Show resolved Hide resolved
website/src/layouts/docs.vue Outdated Show resolved Hide resolved
website/src/components/Sidebar.vue Outdated Show resolved Hide resolved
@nblthree nblthree requested a review from krmax44 May 11, 2019 13:31
Copy link
Contributor

@krmax44 krmax44 left a comment

Choose a reason for hiding this comment

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

So I have refactored some bits to make it cleaner and easier to understand and I tweaked some design aspects. Only remaining issue: when scrolling, the page jumps a bit because scrolling changes the hash and changing the hash changes the scroll position. So we need to decouple that.

@krmax44 krmax44 self-requested a review May 11, 2019 16:21
@nblthree
Copy link
Member Author

nblthree commented May 11, 2019

when scrolling, the page jumps

history.pushState(null, null, hash) will do the job with some other changes

You made some good changes but some I don't like I will wait until @egoist decide what he wants

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.

Website: Change the hash while scrolling
3 participants