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

[do not merge] add a rightSidebar option #1288

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

trusktr
Copy link
Member

@trusktr trusktr commented Jul 16, 2020

Summary

Allows the sidebar to be on the right side. This is something developers might want (f.e. to match UX patterns on other parts of a site).

When sidebar is on the right the navbar and github corner badge are on the left on desktop. No changes to navbar or github corner on mobile (because on mobile they are hidden when the menu is open).

  • This should not be merged until the options I added to index.html files are removed. I added the options so that you can test with vercel. After review, I will remove the options.

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 of default theme, 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 (perhaps not enough tests, but we should settle on testing first, i.e. playwright PR)

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

@trusktr trusktr added enhancement docs related to the documentation of docsify itself do not merge semver-minor This needs a semver-minor release labels Jul 16, 2020
@vercel
Copy link

vercel bot commented Jul 16, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/hkptkpq47
✅ Preview: https://docsify-preview-git-right-sidebar.docsify-core.vercel.app

@trusktr trusktr changed the title add a rightSidebar option [do not merge] add a rightSidebar option Jul 16, 2020
@@ -108,7 +108,7 @@ window.$docsify = {
## hideSidebar

- Type : `Boolean`
- Default: `true`
- Default: `false`
Copy link
Member Author

Choose a reason for hiding this comment

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

This was wrong

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 16, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8e98cb8:

Sandbox Source
docsify-template Configuration

* develop:
  remove `lib` from pull request template as it is gitignored
  fix: gitignore was ignoring folders in src, so VS Code search results or file fuzzy finder were not working, etc
& > ul
padding-inline-start: 0;
padding-inline-end: 40px; // Chrome's default value.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jhildenbiddle I aimed to make all the changes in here non-breaking for the end user.

You may have some opinion on how better to organize this change. Happy to hear if you do!

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for padding-inline-end: 40px; thing is to flip it to the other side when the menu is on the left, otherwise it has a default style in Chrome's user-agent style of padding-inline-start: 40px;.

Maybe padding-inline-start: 40px should be hard-coded for nonright-sidebar` mode, otherwise that one can vary from browser to browser (I don't know if they settled on the same values, haven't checked).

@jhildenbiddle
Copy link
Member

Is the intention to target this for v4, or as a placeholder for v5? If v4, then my questions / concerns outlined in the original issue (#1282) remain:

I think this is a feature best targeted for v5 and discussed in that context.

There's more to consider than just making the menu appear on the right:

  • How are other docsify themes affected?
  • How are third-party themes affected?
  • How are third-party plugins affected (i.e. do any rely on the sidebar-on-left behavior)?
  • If the sidebar slides in from the right, should the sidebar toggle also be located on the right?
  • How does this behavior look at desktop resolutions?

These are just off the top of my head. I'm sure there are more.

More importantly, not every preference needs to be a core docsify feature. Is this feature useful? Possibly, but given the fact that it has never before been requested in the nearly four years that docsify has existed, I think it's safe to say that this is not a high priority feature. I'd look to our existing issues list and feature requests to get a better idea of where we should be focused.

The good news is that folks that want a sidebar on the right can do so today by adding a custom <style> tag to their index.html file with the tweaked values you mentioned above. No need to wait for a new docsify option. We can target this issue for v5, see what kind of activity it gets, and determine if/how to implement this feature in v5 when the time comes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge docs related to the documentation of docsify itself enhancement semver-minor This needs a semver-minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants