Repository navigation
There is an error in opening the mobile page of the onlyCover parameter #1173
Description
Bug Report
Steps to reproduce
- Setting param
window.$docsify = {
onlyCover: true,
}What is current behaviour
Should be removed, Otherwise, when you click it, the problem of Figure 2 will appear.
What is the expected behaviour
Other relevant information
-
Bug does still occur when all/other plugins are disabled?
-
Your OS: mac
-
Node.js version:
-
npm/yarn version:
-
Browser version:
-
Docsify version: 4.11.3
-
Docsify plugins:
Activity
anikethsaha commentedon May 12, 2020
can we keep the burgur icon and fix the sidebar so that when it is clicked, it slides as it suppose to be ?
else we can remove it 👍
sy-records commentedon May 12, 2020
What it does is load only the cover page when you visit the home page.
I'm not sure if I should remove the burgur icon when it exists.
sy-records commentedon May 12, 2020
https://simps.io/ For my site, I think it should be deleted.
sy-records commentedon May 12, 2020
But the sidebar style of the
docsify-dark-modetheme also needs to be fixed.https://docsify.js.org/ website was affected.
anikethsaha commentedon May 12, 2020
ok cool , I didnt notice the
onlyCoveroption. feel free to fix it.go ahead 👍
trusktr commentedon Jun 15, 2020
Makes sense to not show the menu in that case.
I added the
buglabel and put it in theTo docolumn of the4.xproject board. Let's triage issues as we go.mohammedsahl commentedon Jun 17, 2020
This seems like a quick CSS fix. I can work on this. From my understanding, all that needs to be done is remove the burger icon (sidebar-toggle) from the cover when viewed on a mobile device. @sy-records
sy-records commentedon Jun 18, 2020
@mohammedsahl It may be necessary to judge configuration items:
onlyCover: trueWhen
onlyCoveristrue, you can delete the icon on the homepage,but it does not affect the icon on the body.
mohammedsahl commentedon Jun 19, 2020
Done with fixing the bug. Writing up an integration test right now. I should have a PR out pretty soon
mohammedsahl commentedon Jun 20, 2020
The integration test, as it turns out, needs a bit of fiddling around with cypress. While I can push a fix to this issue I don't wanna do so until an integration test is also included, as implied from #1217. What I'm currently working on is setting up the test suit so that multiple configs can be tested, which in-turn, can help others and myself add new tests as required. I suppose it is going to be the same for issue #1233.
Easy fix, but no integration test.
I wanted to know if I should push my changes regardless and shift focus to writing up a suite to test multiple config files right after.
The problem is that I cannot really triage the work required since this is the first time I'll be cypress. But I know for sure that issues #1173 and #1233 are simple enough fixes. I simply want to include a cypress test as this is the ideal practice. Doing that will take some time.
@trusktr @sy-records @anikethsaha
trusktr commentedon Jun 24, 2020
I'm am very in favor of waiting to merge any current and new PRs until they have unit and integration tests in place. It is well-worth the wait IMO.
Do you mean merge the PR? Or push new changes to the PR? I am fine with reviewing integration test improvements in the same PR just to test that change. It's really great to get the tests working better! 👍
No rush. Better to have it take the time it takes to get tests in place for the changes, so that we don't forget about it if we were to merge now without tests.
I just thought about this, and it occurred to me that maybe it would be a breaking change actually. Maybe we should only not show the content below the cover, but we should keep the menu. Any thoughts @jhildenbiddle? Should we put this in 4.x or 5.x?
jhildenbiddle commentedon Jun 24, 2020
Are we sure this is a bug? The docs state the following:
onlyCover
BooleanOnly coverpage is loaded when visiting the home page.
The docs don't mention what the behavior of the menu toggle should be, and I can see an argument both for and against the toggle being displayed when
onlyCoveristrue. Perhaps this is something we don't need to "fix" at all (even if it is different from the previous behavior).Is there a consensus on how we want this feature to behave?
sy-records commentedon Jun 25, 2020
For a better user experience, it should be removed, When clicking on the sidebar, it is blank, which is not friendly.
jhildenbiddle commentedon Jun 25, 2020
Ahh. Now I see, @sy-records. I must have reviewed your site by resizing the browser to mobile size without refreshing, so the issue didn't present itself. When I opened the menu, I saw a fully-rendered sidebar.
Near term, I think we would just restore the previous behavior of hiding the menu toggle when
onlyCoveristrue. This work should be targeted for v4. For v5, I think this feature should be revisited:onlyCoverisfalse) but the current "show only the coverpage until you click something" behavior is implemented via CSS. There are two reasons for this: 1) SEO and 2) performance. Both of these suffer today whenonlyCoveristruebecause 1) the only content for search engines to index is the coverpage, and 2) the entire site is reloaded to navigate anywhere from the coverpage. So long as we render the coverpage before rending the content (which I think is how it works today?), the perceived performance will be better than our current implementation.sy-records commentedon Jun 26, 2020
I didn't modify it, but I tested #1243, It's normal.
I enabled this option because I only want to show the home page and then redirect to the corresponding language document by clicking to jump to another link.
This option should be retained in v5 as well.
trusktr commentedon Jun 26, 2020
If there was something changed, let's make it work like before. I'm not familiar with this feature from before. Was the menu icon always hidden before?
But I agree with @jhildenbiddle's sentiment that it is more valuable to show a menu. The reason is: repeat visitors may know where they want to navigate to and would appreciate getting there quickly. If they only have a "get started" button, it means they have to click that first, then click the menu on the next page to get where they want, which is extra steps compared to just opening the menu an navigating.
sy-records commentedon Jun 26, 2020
It should be left to the user to decide, we only provide features.
and now when
onlyCoveristrue, thesidebaris blank.sy-records commentedon Jun 26, 2020
onlyCover
BooleanOnly coverpage is loaded when visiting the home page.
This icon is only removed when the option is true, it does not affect other functions.
jhildenbiddle commentedon Jun 26, 2020
The project maintainers decide which features to provide, how they function, and what changes (if any) are made from one version to the next. That's what we're discussing.
This text doesn't appear on the website, nor does any other mention about how the
onlyCoveroption affects the menu button toggle. It sounds like the previous behavior was to hide the menu toggle whenonlyCoverwastrueand we are restoring that functionality #1243. How we handle this feature and its functionality moving forward is a separate discussion.Got it. So your intention is to limit user choice in hopes of driving them towards the "Get Started" button where you will redirect them based on some logic (geo-location/IP) to matching site translation? What if your logic doesn't work (for example, I wasn't redirected to
/pt-br/on your site when connecting from Portugal via VPN). What if the visitor's preferred language does not match their current geographical location? Why not continue to redirect as your do today along with a sidebar menu that displays a list of translations to choose from like docsify.js.org?sy-records commentedon Jun 26, 2020
Yes, we should discuss the development of
onlyCoverseparately.If can't get a match, of course there will be a default way to handle it, which is not something we should be concerned about.
It's just that the home page doesn't show up, and the sidebar is still displayed after the jump.
when
onlyCoveristrue, The home page sidebar is blank, why not remove it?sy-records commentedon Jun 26, 2020
I deployed #1243 and you can experience it.
https://simps.io
trusktr commentedon Jun 26, 2020
Continued in #1249, with an idea for an option to have menu with onlyCover.