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($theme-default): support darkmode (close #1865) #2301

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat($theme-default): support darkmode (close #1865) #2301

wants to merge 4 commits into from

Conversation

Mister-Hope
Copy link
Contributor

@Mister-Hope Mister-Hope commented Apr 13, 2020

Summary

Add darkmode feature to @vuepress/theme-default.

Meanwhile, moving some variables to @vuepress/core/client/styles/config.styl

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

  • Feature

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

  • Yes

If user is using dark theme on their device, darkmode will be enabled automatically.

You have tested in the following browsers:

  • Chrome 80
  • Firefox
  • Safari
  • Edge 80
  • 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

This was referenced Apr 13, 2020
@Mister-Hope Mister-Hope changed the title feat($theme-default): support darkmode feat($theme-default): support darkmode (close #1865) Apr 13, 2020
@Mister-Hope
Copy link
Contributor Author

Could anyone review this PR? I believe it's an amazing feature. Nearly half a month has passed, you can just release 1.5.0 with it along with the fix #2339 @newsbielt703 @meteorlxy

@bencodezen
Copy link
Member

@Mister-Hope Have you verified if this will work in IE11? It would be ideal for us to avoid having the theme break due to incompatibility with CSS variables.

@bencodezen bencodezen self-requested a review May 11, 2020 11:09
Copy link
Member

@bencodezen bencodezen left a comment

Choose a reason for hiding this comment

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

@Mister-Hope Thanks for your work on this. The main questions that need to be answered for this theme include:

  1. Does it work on IE11 and other browsers?
  2. Does it pass accessibility checks?

I know that the work involved for these parts are fairly involved. So at this time, if you'd like to have your work shared with others, I would recommend creating a separate atomic theme that other people can use since it may take some time before this can be integrated as an official dark theme.

@Mister-Hope
Copy link
Contributor Author

@Mister-Hope Thanks for your work on this. The main questions that need to be answered for this theme include:

  1. Does it work on IE11 and other browsers?
  2. Does it pass accessibility checks?

I know that the work involved for these parts are fairly involved. So at this time, if you'd like to have your work shared with others, I would recommend creating a separate atomic theme that other people can use since it may take some time before this can be integrated as an official dark theme.

  1. It do drop IE support, thanks reminding, I will add it.

  2. Is there any checkes now in this repo? I only edit the css files, I did not remove any aira-related label or change any html tags. So why wound it break the accessibility you have?

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented May 11, 2020

I know that the work involved for these parts are fairly involved. So at this time, if you'd like to have your work shared with others, I would recommend creating a separate atomic theme that other people can use since it may take some time before this can be integrated as an official dark theme.

Since last year september, I feel like your whole team is on vacation ( You could not deny it), I even open a issue #2144 . More and more people give up using it. Also, I post my theme to awesome-vuepress , but ulivz is totally disappeared, that means I donot have a platform to post my theme. I like this project, so I spend a lot of time developing a good doc and blog theme. It has the most features among all the theme I known. Besides, as far as I know, there are plenty of people willing to have this darktheme, so I spend time make one. If you can contact ulivz, I really wish you can request him to transfer awesome-vuepress under vuepress-community, at least meteorlxy is active. @bencodezen

@bencodezen
Copy link
Member

@Mister-Hope That must have been frustrating.

Onboarding the new team has certainly been a challenge and we are looking to make changes to the process in order to reduce issues like you mentioned.

I have reached out to Ulivz in hopes of getting access to the repos since unfortunately it looks like none of us have access to keep those updated. Once we do, I will make sure those are kept up to date as well.

In the meantime, you raise excellent points that I intend on addressing.

For now, don't worry about the accessibility checks since we will need a full audit on both official themes; but to your point, it would be better to give people access to a dark theme so they have somewhere to start.

Could you make sure that the checks pass IE11? My main concern is the CSS variables which I don't believe is supported.

@Mister-Hope
Copy link
Contributor Author

I will add ie11 support these days

@bencodezen
Copy link
Member

@Mister-Hope Sounds great. Thank you for your patience and I look forward to getting your PR merged soon!

Copy link
Member

@ulivz ulivz left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, but following CSS is a better and lightweight implementation.

body {
  filter: invert(100%) hue-rotate(180deg);
}

@zefrieira
Copy link

Hi @Mister-Hope

Do you plan to keep working in this PR? I'm very interested in it.

Maybe you could release this as a standalone theme

@Mister-Hope
Copy link
Contributor Author

Hi @Mister-Hope

Do you plan to keep working in this PR? I'm very interested in it.

Maybe you could release this as a standalone theme

I have a theme called vuepress theme hope

@kefranabg
Copy link
Collaborator

@Mister-Hope Do you need help on this?

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Jul 14, 2020

@Mister-Hope Do you need help on this?

Yes, please. I am busy now,I am planning to add a fallback color before all the css vars and I think it will works fine on IE.

@CieNTi
Copy link

CieNTi commented Aug 23, 2020

@Mister-Hope Thanks for all your effort, it is a pity this feature is not still there

@wxsms
Copy link
Member

wxsms commented Oct 26, 2020

Thanks for your contribution, but following CSS is a better and lightweight implementation.

body {
  filter: invert(100%) hue-rotate(180deg);
}

lightweight for sure. but 100% not better.

@2xAA
Copy link

2xAA commented Nov 28, 2020

Thanks for your contribution, but following CSS is a better and lightweight implementation.

body {
  filter: invert(100%) hue-rotate(180deg);
}

This really isn't a better implementation.

Emoji and images will have their color inverted.

Images can be corrected by filtering and rotating the hue again, but this is wasteful on the client's resources, especially for older machines.

Try scrolling on a longer Vuepress page on any older retina Mac with this solution, Chrome will have a hard time keeping up with the page rendering.

Applying this CSS to just the body will result in shorter pages displaying white at the bottom of the screen. Targeting the html tag instead works for Chrome and Safari, but Firefox behaves differently, showing white at the bottom of shorter pages still.

Not the reply I'd expect to see from a core team member.

I don't think this PR is the solution, but substantial work should be done to build a vuepress theme which respects the color scheme of the client machine and not just patched with a quick fix.

@lgg
Copy link

lgg commented Jan 27, 2021

@Mister-Hope @kefranabg hello, thank you for your work. Any updates for now?

@Mister-Hope
Copy link
Contributor Author

Nope, V2 may support this.

now you can try my theme called vuepress-theme-hope

@lgg
Copy link

lgg commented Jan 27, 2021

@Mister-Hope will you add IE11 support to match merge requirements from the reviewer?

@Mister-Hope
Copy link
Contributor Author

@Mister-Hope will you add IE11 support to match merge requirements from the reviewer?

to be honest, I am not pretty interested now.

As you can see, no core contributor is maintaining V1 now, they are just reviewing PRS, and even you turn to vitepress.

Also, V2 version is nearl complete by meteorlxy. And he is planing to support darkmode.

@lgg
Copy link

lgg commented Jan 27, 2021

@Mister-Hope what is vitepress? And where I can found v2?

@meteorlxy
Copy link
Member

@Mister-Hope Would you like to transform this PR to v2?

@Mister-Hope
Copy link
Contributor Author

@Mister-Hope Would you like to transform this PR to v2?

I would like to,but I think a full rebuilt of style would be better.(dropping stylus)

@meteorlxy
Copy link
Member

Yep, just like what we discussed before.

I was considering to drop the v1 layout system, and only support one Layout and one NotFound like v0 & vitepress. That was the reason why I didn't suggest to refactor the style in a rush. But now I still can't determine if that is necessary, so we'd better not be stuck with it, and go ahead to refine our current theme & plugins.

@Mister-Hope
Copy link
Contributor Author

Since I am using plugin-blog, I would prefer to remain the layout system. Otherwise the Layout.vue would be too complicated.

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.