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

fix($theme-default): navbar dropdown issue (fix #2349) #2339

Closed
wants to merge 3 commits into from
Closed

fix($theme-default): navbar dropdown issue (fix #2349) #2339

wants to merge 3 commits into from

Conversation

Mister-Hope
Copy link
Contributor

Summary

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

  • Bugfix

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

  • Yes
  • No

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

@Mister-Hope Mister-Hope changed the title fix theme-default navbar dropdown issue fix($theme-default): navbar dropdown issue Apr 25, 2020
@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Apr 25, 2020

Sorry for the last fix, I was busy writing my undergraduate thesis last week. @newsbielt703 And this fix really takes me hours to have a simple and correct fix.

Since it is not a small bug, you can release 1.4.2 with this fix after review and test it.

Besides, if you can review the darkmode I provided, you can also release 1.5.0,

@Mister-Hope Mister-Hope changed the title fix($theme-default): navbar dropdown issue fix($theme-default): navbar dropdown issue (fix #2349) Apr 29, 2020
@guojie
Copy link

guojie commented Apr 30, 2020

为啥我升级到1.4.1还是不行呢

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Apr 30, 2020

为啥我升级到1.4.1还是不行呢

Vupress team members all have their own works to do, so no one is reviewing this PR, this PR is the fix but it's not merged or published. @meteorlxy @newsbielt703 Could you spend some time review this PR?

@@ -75,13 +76,18 @@ export default {
},

props: {
inSidebar: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'll only display in sidebar if the device is mobile, so it seems that you only need one of them (inSidebar or isMobile).

@@ -2,12 +2,13 @@
<div
class="dropdown-wrapper"
:class="{ open }"
@focusout="close(true)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The close method will only really close when the arg is false or the device is mobile, and it will never happen here.

mounted () {
this.isMobile = 'ontouchstart' in document.documentElement
},

methods: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The methods isn't intuitive. The close won't really close, and what isButton? I believe it's here to detect whether users use keyboard to click the button, right?

@billyyyyy3320
Copy link
Collaborator

billyyyyy3320 commented May 2, 2020

I've make some improvement, see #2365. Welcome to review.

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.

None yet

3 participants