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

Default theme override structure #571

Closed
FistMeNaruto opened this issue Jun 11, 2018 · 4 comments
Closed

Default theme override structure #571

FistMeNaruto opened this issue Jun 11, 2018 · 4 comments

Comments

@FistMeNaruto
Copy link
Contributor

FistMeNaruto commented Jun 11, 2018

Feature request

What problem does this feature solve?

Currently, the override.styl is unable to override style declarations properly, only variable overrides work as they should. This is due to config.styl being required at the top of most .styl files. This prevents override.styl declarations from working, since they are outputted before the default theme styles. This structure also has a side effect of repeating the style declarations (because they are required multiple times)

Here is an example:
override.styl

Output for .content code element
.content code element

How should this be implemented in your opinion?

I would suggest implementing 2 override files (styleOverride and variableOverride ? ). Keep variableOverride import at the bottom of config.styl and add styleOverride import at the bottom of theme.styl file. This would allow to override both variable and style declarations. To prevent breaking changes, variableOverride could be kept named override.styl

While webpack does a good job of stripping the repeated styles in production build, I don't see a reason to @require './config.styl' in all the files. Requiring it once at the top of theme.styl should be sufficient, right?

EDIT: After inspecting this a bit more, the repeated style declarations apparently come from theme components, as most of them @require the config.styl

Are you willing to work on this yourself?**

Yes, after getting a permission.

@LinFeng1997
Copy link
Contributor

LinFeng1997 commented Jun 18, 2018

Maybe you can use !important to solve your problem.or use vuepress eject [targetDir] to overrider default theme?

@FistMeNaruto
Copy link
Contributor Author

Ofcourse I can, both !important and ejecting would work, I am just offering to fix the underlying structure of the default theme so the !important (which should be avoided) and ejecting (which disables future theme updates) would not be necessary :)

@FistMeNaruto
Copy link
Contributor Author

Can anyone confirm 2f53f2f fixed this issue? If so, it can be closed.

@ulivz
Copy link
Member

ulivz commented Aug 20, 2018

@FistMeNaruto Yes, it has been fixed in f998802

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

No branches or pull requests

3 participants