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

feat: darkmode (close #1865) #2232

Closed
wants to merge 4 commits into from
Closed

feat: darkmode (close #1865) #2232

wants to merge 4 commits into from

Conversation

Mister-Hope
Copy link
Contributor

@Mister-Hope Mister-Hope commented Mar 18, 2020

Summary

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

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:
image
image

It should fix issue #1865

@Mister-Hope Mister-Hope mentioned this pull request Mar 18, 2020
@Mister-Hope Mister-Hope changed the title feat: darkmode feat: darkmode (close #1865) Mar 18, 2020
@Mister-Hope
Copy link
Contributor Author

As KieranHunt reminds, I will check the switch license which I used in the PR.

@ChungZH
Copy link
Contributor

ChungZH commented Mar 21, 2020

I think it's too dark 😂
Maybe gray will be better?

@Mister-Hope
Copy link
Contributor Author

I think it's too dark
Maybe gray will be better?

Yes, I will make some changes

@Mister-Hope
Copy link
Contributor Author

@ChungZH Color is changed
@KieranHunt Switch is changed

I am now working on prefers-color-scheme feature

@Mister-Hope
Copy link
Contributor Author

New outlook
image
image

It's not pure back any more

@ChungZH
Copy link
Contributor

ChungZH commented Mar 22, 2020

Cool!

@ArsalaBangash
Copy link

This looks great! I've been looking for an dark mode solution for my university's Hacklab Website.

I saw that you placed the switch in a similar location on your personal website. I would prefer the switch to be at the very end.

Is there a reason you placed the switch next to the search bar?
Could the plugin be configured so that you can choose?

@Mister-Hope
Copy link
Contributor Author

This looks great! I've been looking for an dark mode solution for my university's Hacklab Website.

I saw that you placed the switch in a similar location on your personal website. I would prefer the switch to be at the very end.

Is there a reason you placed the switch next to the search bar?
Could the plugin be configured so that you can choose?

Good advice, I can changed it to the bottom of the page.

@meteorlxy
Copy link
Member

image

<script>
export default {
data: () => ({
isDarkmode: { type: Boolean, default: false }
Copy link
Member

Choose a reason for hiding this comment

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

What's this for? It looks like props style

$nightBgcolor ?= #1e1e1e
$nightFontcolor ?= #9e9e9e
$nightBordercolor ?= #151310
$contentClass = '.theme-default-content'
Copy link
Member

Choose a reason for hiding this comment

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

Configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Configurable?

Yes

Copy link
Member

Choose a reason for hiding this comment

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

Well, I was asking why not use $contentClass ?= to avoid override issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I was asking why not use $contentClass ?= to avoid override issues

The default theme is using = instead of ?=

Copy link
Member

Choose a reason for hiding this comment

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

BTW, it's already a pre-defined value in theme default

@@ -0,0 +1,9 @@
.theme-dark
// 主页
Copy link
Member

Choose a reason for hiding this comment

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

No chs comments

@@ -22,3 +22,7 @@ pre.vue-container
color #808080
font-weight light


.theme-dark
Copy link
Member

Choose a reason for hiding this comment

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

only theme-vue has bit-sponsor class

@meteorlxy
Copy link
Member

  1. A standalone plugin should work well regardless of what the theme is. I'm not sure if this plugin can work with all (or most of) other themes. If it's mainly for the default theme, I think it's better to integrate into theme-default.

  2. If you want to make a standalone darkmode plugin, I suggest to publish it as a community plugin, as we are not going to maintain more plugins in official repo.

Anyway, good job and thanks for contribution 👍

@Mister-Hope
Copy link
Contributor Author

I have completed building and testing this feature with prefer-color-scheme on my theme based on css vars, and I will open a new PR to ship this feature directly to the offical @vuepress/theme-default.

@Mister-Hope
Copy link
Contributor Author

I have opened a new PR #2301

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