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

Updated docs site dark and light mode with switch and redesigned search bar using docsify-darklight-theme #1182

Merged
merged 4 commits into from
May 22, 2020

Conversation

boopathikumar018
Copy link
Contributor

@boopathikumar018 boopathikumar018 commented May 17, 2020

Summary

What kind of change does this PR introduce? (check at least one)

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

If changing the UI of default theme, please provide the before/after screenshot:

Updated docs site dark and light mode with switch and redesigned search bar using docsify-darklight-theme you can refer the docs and see full preview here

Light Mode

before

image

after

image

Dark Mode

before

image

after

image

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

  • Yes
  • [x ] 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.)

  • [x ] Chrome
  • [x ] Firefox
  • Safari
  • [ x] Edge
  • IE

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

  • [ x] A convincing reason for adding this feature
    • Default theme mode detection in supported browser versions.
    • Theme Switcher.
    • Themes are customizable based on your color preferences.
    • Option for other plugins to support for (Dark/Light) mode. View setup guide here.
    • Preferences can be modified directly in window.$docsify configuration object.
    • Toogle icons can be configured based on your preference using configuration object.
    • Default theme(Dark/Light) can be configured based on your needs.
    • Themes are remembered and retrieved from local storage.
    • Redesigned search box.
  • [x ] Related documents have been updated
    Documentation & Preview
  • 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:


  • DO NOT include files inside lib directory.

Copy link
Member

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Can we have the search bar not changed?

also,please fix the CI issue.

other looks good.

@anikethsaha
Copy link
Member

also, can we have the old font ?

@boopathikumar018
Copy link
Contributor Author

Can we have the search bar not changed?

are you sure you want to have old search bar ?

also,please fix the CI issue.

Can you please explain this in detail

also, can we have the old font ?

sure!

@anikethsaha
Copy link
Member

are you sure you want to have old search bar ?

yes, that's the one shipped by default. So we need to show that.

@boopathikumar018
Copy link
Contributor Author

yes, that's the one shipped by default. So we need to show that.

Yes I will change it to default style.

@boopathikumar018
Copy link
Contributor Author

Requested changes completed

  • Changed to default search bar
  • Updated font-family

Dark Mode

image

Light Mode

image

@anikethsaha anikethsaha reopened this May 20, 2020
@vercel vercel bot temporarily deployed to Preview May 20, 2020 07:20 Inactive
@anikethsaha anikethsaha reopened this May 20, 2020
@anikethsaha anikethsaha reopened this May 20, 2020
@anikethsaha anikethsaha merged commit 415f295 into docsifyjs:develop May 22, 2020
@trusktr
Copy link
Member

trusktr commented Jun 8, 2020

that new search bar was neat, but better to keep the default (non-breaking change) for now.

It would be great to provide style addons though. Would be happy to accept a PR that can add (for example) a CSS file that someone can optionally import to have an alternative search bar (or theme), etc.

Also take a look at docsify-themeable by @jhildenbiddle. That extension makes great use of CSS variables for theme customization. I think it would be great to have a system like that in core (but with the default theme), and then @jhildenbiddle's theme can be an additional (optional) theme that builds on top of that system.

Maybe each theme can have its own variables. There's some stuff in docsify-themeable that would be great in core by default, while some stuff (like the actual theme) would be great as an optional theme file.

@anikethsaha
Copy link
Member

did you meant this search bar ? #1182 (comment)

@trusktr
Copy link
Member

trusktr commented Jun 8, 2020

I meant the one with rounded corners in the "after" shots. But the new dark one looks great too.

@anikethsaha
Copy link
Member

rounded one was the proposed one, I asked for the old one. do you prefer the rounded one ?

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Jun 8, 2020

Please hold off on this merge. I will provide more detail before the end of the day. Thanks!

Looks like this has already been merged, unfortunately. I'll provide more information before the end of the day for why we should revert this change.

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Jun 9, 2020

@anikethsaha, @trusktr --

  1. Generally speaking, we should be cautious about which (if any) third-party docsify-related addons we use on the public docsify site. Most visitors will view the site as an example of what they will get with an "out of the box" docsify installation. If after following our installation guide their site looks or behaves differently (which will be the case if docsify.js.org uses this theme), confusion can lead to issues being filed (which we have to manage) and an overall frustrating experience. We could try to address this by listing all of the plugins we use on the site at the end of our installation guide, but then we're essentially endorsing some addons over others which isn't great for the community. Unless there is legitimate need, I believe it's best to let docsify.js.org be a showcase primarily for code from the docsify team.
  2. Aside from the general guidance above, the docsify site should not use a third-party theme due to the maintainability issues it creates. Consider what will happen when a docsify UI change is necessary that requires HTML and/or CSS updates. The docsify team can modify the generated HTML, but they will now be reliant on the third-party theme author to update their CSS before changes can be made public. This is not a position we want to put ourselves in, especially when our own documentation lists four officially-supported themes that the docsify team is presumably responsible for maintaining (but they don't actually use?).
  3. If the goal is to stabilize v4 and turn our attention to v5, changes like this (new features or non-critical updates) should not be merged. Doing so only increases the amount of v4 testing that needs occur and/or the likelihood that new v4 issues will appear, further delaying the release of v5. For example:
    • This theme does not render properly in IE11.
      IE11 CSS
    • This theme's plugin JS throws an error in IE11 (officially supported by docsify v4) which breaks all docsify-powered sites.
      IE11 JS Error
    • This theme/plugin is not compatible with the theme preview links on docsify.js.org
    • On small screens (e.g. phones), the toggle switch is visible but positioned under the transparent portion of the Github corner image. This makes it impossible to toggle themes and unexpectedly navigates to Github when attempting to click the toggle.
    • ... and maybe more (but also maybe not).

I think @boopathikumar018 has done a nice job with docsify-darklight-theme, so I don't want the above to sound discouraging. My feedback is less about this particular theme/plugin combo and more about what's best for docsify (IMHO, anyway). If a better dark/light theme switcher is what we're after (and that is how this PR was pitched), I suggest the following:

  1. I'd love to see @boopathikumar018 offer a more focused plugin (not a theme) that provides dark/light theme functionality with two core features: the ability to specify separate dark and light URLs (so dark/light themes are configurable) and the ability to enable a dark/light URL by default based on the operating systems current dark/light mode settings (see here for details). The only CSS that should accompany this plugin are the styles necessary for the toggle itself.
  2. An interesting stretch goal for the plugin would be to allow more than two theme URLs to be specified (not just dark/light). When two URLs are specified, the icon element acts as a toggle. When three-or-more URLs are specified, a drop-down list of themes appears under the icon on hover/tap. I mention this because both the docsify and docsify-themeable sites could use this functionality to provide theme previews.
  3. Ensure the JS errors have been addressed in IE11. I didn't spend too much time debugging, but it looks like an arrow function is used which IE (obviously) doesn't like.
  4. Provide the current docsify-darklight-theme "dark" theme as a separate theme (no JS, just the theme). This will allow users to select it as their theme without the plugin, or use it with the plugin along with docsify's own "vue.css" theme to recreate docsify-darklight-theme's current look and functionality.

The advantage of the steps above comes down to flexibility: both docsify and docsify-themeable could use the plugin to swap between their own "official" dark/light themes (vue.css/dark.css for docsify, "simple" and "simple-dark" for docsify-themeable). More importantly, users would be able to select their own dark/light themes, perhaps mixing themes from different authors.

@anikethsaha
Copy link
Member

I didnt test that in IE11. (Sorry, It was my bad )
I think we should revert this if that's not working (which is not as mentioned above. )

Also i agreed with the docs site as a point of reference by docsify users.

IMO, let's remove any dark mode plugin in the official docs. We have planned to keep it officially from the core in the next version (v5), so I don't think we should have it now.

@boopathikumar018 You did an amazing work with the plugin, but as mentioned by @jhildenbiddle , your plugin is changing the default theme which we should not have as this is one of the active examples of docsify users.

I am reverting this and will remove the old dark mode plugin as well.

anikethsaha added a commit that referenced this pull request Jun 9, 2020
… redesigned search bar using docsify-darklight-theme (#1182)"

This reverts commit 415f295.
anikethsaha added a commit that referenced this pull request Jun 9, 2020
…ned search bar using docsify-darklight-theme" (#1207)

* Revert "update: updated docs site dark and light mode with switch and redesigned search bar using docsify-darklight-theme (#1182)"

This reverts commit 415f295.

* docs: removed the old dark mode as well
@trusktr
Copy link
Member

trusktr commented Jun 10, 2020

I didnt test that in IE11. (Sorry, It was my bad )

It'd be neat to hook up a test system that runs for each pull request that can lets us see/test how the site works in various browsers. I see in some PRs not all browsers are tested (the check boxes from the PR template are not checked). It is difficult for someone in macOS, for example, to test IE 11.

I found some tools online, and some of them (if not all) have a free-for-open-source option:

It'd be super sweet to click a link (generated by a bot) in a pull request that takes you directly to the live site in a specific browser.

@jhildenbiddle How do you currently test in IE11?

let's remove any dark mode plugin in the official docs. We have planned to keep it officially from the core in the next version (v5), so I don't think we should have it now... will remove the old dark mode plugin as well.

@anikethsaha Why remove it? Shouldn't we keep it, then as @jhildenbiddle mentioned it will be toggleable with a switch?

@trusktr
Copy link
Member

trusktr commented Jun 10, 2020

@boopathikumar018 Would you be interested in implementing the style toggle switch like @jhildenbiddle mentioned above? Basically,

  • make a toggle switch (maybe it toggles a light/dark class on the body and nothing more).
  • make a docsify config option to provide light/dark theme
  • the toggle switch swaps between the specified themes.

Then your new/improved dark theme could be an option that people can opt into by specifying it with the new config option.

@anikethsaha
Copy link
Member

the old one was buggy as well. so I removed both

@anikethsaha
Copy link
Member

I would prefer a dark mode from core instead of plugin.

@trusktr
Copy link
Member

trusktr commented Jun 10, 2020

from core instead of plugin.

What do you mean "from core" vs "from plugin"? Styles are just CSS files (currently). I think the new feature that @jhildenbiddle suggests would simply let users list which style files to use for light and dark. We might have default values for those.

But I don't see why to delete them if we can just fix the bugs.

@anikethsaha
Copy link
Member

anikethsaha commented Jun 10, 2020

What do you mean "from core" vs "from plugin"?

Like without plugin as the sidebar, other navbar items are implemented.
(not a blocker tho)

But I don't see why to delete them if we can just fix the bugs.

Yea we can, also needs to change the styling for that as well if we are considering that cause it is not so good looking.

Note: the old dark mode was not working properly as respect to styling with other themes we provide.

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Jun 11, 2020

Since docisfy-themeable offers a light and dark theme already, these will be available as part of v5.

"Simple" Theme

theme-simple-content

"Simple Dark" Theme

theme-simple-dark-content

Users can select one of these themes, or they can include both stylesheets and leverage the prefers-color-scheme media query to have supporting operating systems select the light/dark stylesheet that matches their OS mode (I covered this in detail at jhildenbiddle/docsify-themeable#19):

<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/docsify-themeable@0/dist/css/theme-simple.css">
<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/docsify-themeable@0/dist/css/theme-simple-dark.css" media="(prefers-color-scheme: dark)">

We also have the option of simplifying this setup for users by offering light/dark theme selection as part of their docsify configuration. Docsify can then inject the necessary <link> elements with the appropriate attributes.

<script>
  window.$docsify = {
    lightTheme: 'https://path/to/light-theme.css',
    darkTheme: 'https://path/to/dark-theme.css'
    // ... etc.
  }
</script>

No toggle necessary (for operating systems that support it, which means the latest versions of Windows and macOS), and I'd argue this is what "dark mode" users expect (why make them manually toggle when they have already indicated "I prefer dark mode" via their OS?). A toggle might be a nice option for those on older operating systems that don't offer a "dark mode", but I'm not convinced it needs to be core docsify functionality. Seems much better a plugin, IMHO.

This was referenced Jun 11, 2020
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.

4 participants