-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
refactor: refactoring hideSidebar
configuration.
#1396
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/3kbn0qg3v |
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 2ac291e:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a much better approach than the current one.
cc @sy-records , this will fix #1392 as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write tests for this if possible ?
yup, I will check the test tutorial and try to get a new test. 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add test and we may add more tests of configurations here.
The CI issues cuz the test site includes search plugin
to cause the error.
Error due to docsify-plugin-carbon plugin, @waruqi Can you fix it in your plugin? |
* fix: Can't search homepage content * fix: when pathNamespaces does not exist * add test * update test to fix windows ci
f3709ec
to
23c44ee
Compare
@waruqi thx ur input ! IMHO, literally, it is not ur duty to make the plugin which is based on sidebar to suit the no sidebar case, it seems awkward. |
cc @anikethsaha @sy-records I fixed the CI issue, review it again plz. |
document.querySelector('section.content').style.right = 'unset'; | ||
document.querySelector('section.content').style.left = 'unset'; | ||
document.querySelector('section.content').style.position = 'relative'; | ||
document.querySelector('section.content').style.width = '100%'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just keep this configurations without adjusting much styles right now (dont let render code including much styles), and if there has much users proposal to make it that we can refine it again.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep them and not change them in the z versions.
This comment has been minimized.
This comment has been minimized.
Add |
@sy-records sorry, I can't make it reproducible, could u please do me a favor that checking about it. |
Well, ignore it. I can't reproduce it either. |
what does the |
@Koooooo-7 -- Is this PR still worth considering or should we close it? I admit to not reviewing it due to the age, but I'm happy to if you'd like me to. |
It seems outdate, I'd like to close it for now, |
Refactor
hideSidebar
that removing thetpl
of sidebar (ifhideSidebar : true
).Checking the
hideSidebar
when having options onsidebar
.Summary
What kind of change does this PR introduce? (check at least one)
If changing the UI of default theme, please provide the before/after screenshot:
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
fix #xxx[,#xxx]
, where "xxx" is the issue number)You have tested in the following browsers: (Providing a detailed version will be better.)
If adding a new feature, the PR's description includes:
To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.