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

There is an error in opening the mobile page of the onlyCover parameter #1173

Closed
1 task
sy-records opened this issue May 12, 2020 · 22 comments · Fixed by #1243
Closed
1 task

There is an error in opening the mobile page of the onlyCover parameter #1173

sy-records opened this issue May 12, 2020 · 22 comments · Fixed by #1243
Assignees
Labels
bug confirmed as a bug

Comments

@sy-records
Copy link
Member

Bug Report

Steps to reproduce

  1. 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.

image

image

What is the expected behaviour

image

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:

Please create a reproducible sandbox

Edit 307qqv236

Mention the docsify version in which this bug was not present (if any)

@anikethsaha
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

https://simps.io/ For my site, I think it should be deleted.

@sy-records
Copy link
Member Author

But the sidebar style of the docsify-dark-mode theme also needs to be fixed.
https://docsify.js.org/ website was affected.

@anikethsaha
Copy link
Member

ok cool , I didnt notice the onlyCover option. feel free to fix it.

But the sidebar style of the docsify-dark-mode theme also needs to be fixed.
https://docsify.js.org/ website was affected.

go ahead 👍

@sy-records sy-records mentioned this issue Jun 14, 2020
15 tasks
@trusktr trusktr added the bug confirmed as a bug label Jun 15, 2020
@trusktr
Copy link
Member

trusktr commented Jun 15, 2020

Makes sense to not show the menu in that case.

I added the bug label and put it in the To do column of the 4.x project board. Let's triage issues as we go.

@mohammedsahl
Copy link
Contributor

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
Copy link
Member Author

@mohammedsahl It may be necessary to judge configuration items: onlyCover: true

When onlyCover is true, you can delete the icon on the homepage,

image

but it does not affect the icon on the body.

image

@mohammedsahl
Copy link
Contributor

Done with fixing the bug. Writing up an integration test right now. I should have a PR out pretty soon

@mohammedsahl
Copy link
Contributor

mohammedsahl commented 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
Copy link
Member

trusktr commented 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.

I wanted to know if I should push my changes

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.

Makes sense to not show the menu in that case.

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
Copy link
Member

jhildenbiddle commented Jun 24, 2020

Are we sure this is a bug? The docs state the following:


onlyCover

  • type: Boolean

Only 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 onlyCover is true. 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
Copy link
Member Author

For a better user experience, it should be removed, When clicking on the sidebar, it is blank, which is not friendly.

@jhildenbiddle
Copy link
Member

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 onlyCover is true. This work should be targeted for v4. For v5, I think this feature should be revisited:

  • Do we want to offer this option in v5? I'd argue there is little value in preventing users from navigating the site (because the menu toggle is hidden) or using the most intuitive mechanism for displaying more information (scrolling) aside from forcing them to "look at my beautiful cover page". If someone has a counter argument for how this improves the user experience, I'm all ears.
  • If we do want to offer this option, the implementation should be changed so that all content is loaded (just like when onlyCover is false) 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 when onlyCover is true because 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
Copy link
Member Author

sy-records commented Jun 26, 2020

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.

I didn't modify it, but I tested #1243, It's normal.
image

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
Copy link
Member

trusktr commented 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
Copy link
Member Author

It should be left to the user to decide, we only provide features.

and now when onlyCover is true, the sidebar is blank.

image

@sy-records
Copy link
Member Author

sy-records commented Jun 26, 2020


onlyCover

  • type: Boolean

Only 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
Copy link
Member

jhildenbiddle commented Jun 26, 2020

It should be left to the user to decide, we only provide features.

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 icon is only removed when the option is true, it does not affect other functions.

This text doesn't appear on the website, nor does any other mention about how the onlyCover option affects the menu button toggle. It sounds like the previous behavior was to hide the menu toggle when onlyCover was true and we are restoring that functionality #1243. How we handle this feature and its functionality moving forward is a separate discussion.

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.

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?

Screen Shot 2020-06-26 at 1 06 35 AM

@sy-records
Copy link
Member Author

Yes, we should discuss the development of onlyCover separately.

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.

image

when onlyCover is true, The home page sidebar is blank, why not remove it?

image

@sy-records
Copy link
Member Author

I deployed #1243 and you can experience it.

https://simps.io

@trusktr
Copy link
Member

trusktr commented Jun 26, 2020

Continued in #1249, with an idea for an option to have menu with onlyCover.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug confirmed as a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants