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

back2top button on mobile, sidebar item margin and sidebar exturl link color #684

Merged
merged 6 commits into from
Mar 16, 2019

Conversation

stevenjoezhang
Copy link
Contributor

@stevenjoezhang stevenjoezhang commented Mar 14, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines.
  • Tests for the changes was maked (for bug fixes / features).
    • Muse | Mist have been tested.
    • Pisces | Gemini have been tested.
  • Docs in NexT website have been added / updated (for new features).

PR Type

What kind of change does this PR introduce?

  • Bugfix.
  • Feature.
  • Code style update (formatting, local variables).
  • Refactoring (no functional changes, no api changes).
  • Build related changes.
  • CI related changes.
  • Documentation content changes.
  • Other... Please describe:

What is the current behavior?

屏幕快照 2019-03-17 上午1 04 54

Issue resolved: #482

What is the new behavior?

Fixed 3 bugs:

屏幕快照 2019-03-17 上午1 05 00

  • Link to demo site with this changes: N/A

How to use?

In NexT _config.yml:

...

Does this PR introduce a breaking change?

  • Yes.
  • No.

@1v9 1v9 removed their request for review March 14, 2019 11:25
@stevenjoezhang stevenjoezhang changed the title Fix #482: b2t on mobile Fix #482: b2t on mobile and sidebar item margin Mar 15, 2019
@stevenjoezhang stevenjoezhang changed the title Fix #482: b2t on mobile and sidebar item margin Bugfix: b2t on mobile (#482), sidebar item margin and sidebar link color Mar 15, 2019
Copy link
Member

@ivan-nginx ivan-nginx left a comment

Choose a reason for hiding this comment

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

Need some changes here.


Also, when u create visual fixes like this, make and upload screens please. Sometimes it's hard for understanding where was makes changes.

@@ -89,8 +89,7 @@ $(document).ready(function() {
var display = CONFIG.page.sidebar;
if (typeof display !== 'boolean') {
// There's no definition sidebar in the page front-matter
var isSidebarCouldDisplay = CONFIG.sidebar.display === 'post'
|| CONFIG.sidebar.display === 'always';
var isSidebarCouldDisplay = CONFIG.sidebar.display === 'post' || CONFIG.sidebar.display === 'always';
Copy link
Member

Choose a reason for hiding this comment

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

This will brake ESLinter under the NexT.. Revert this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure


&:hover { color: $black-deep; }
}
.exturl:hover { border-bottom-color: $black-deep; }
Copy link
Member

Choose a reason for hiding this comment

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

span.exturl (for fater element finding)

@@ -15,6 +15,9 @@
border-bottom-color: $black-light;
&:hover { color: $gainsboro; }
}
.exturl:hover {
Copy link
Member

Choose a reason for hiding this comment

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

span.exturl (for fater element finding)

@ivan-nginx
Copy link
Member

ivan-nginx commented Mar 16, 2019

No need to take your time on labels «10px» / «15px». 😄 Just make screens during you make your changes and upload them directly from clipboard buffer here in GitHub. This will take minimal time.

@ivan-nginx ivan-nginx added this to the v7.1.0 milestone Mar 16, 2019
@stevenjoezhang
Copy link
Contributor Author

Maybe this step also needs to be automated. 😂 For example, there are 4 different components in the sidebar, each one can be enabled/disabled, then there are 2^4=16 combinations in total... need to make sure they are all suitable, and it takes time for human to test.

@ivan-nginx
Copy link
Member

And please, create labels instead of that:

Bugfix: b2t on mobile (#482), sidebar item margin and sidebar link color**

Like this:

Fixed back2top on mobile & modified sidebar item margin and sidebar extlurl color**.

Titles like that already completed for simple pasting into the releases notes and to the NexT site, without needed to modify them.

If back2top → it's already good to write it as option:

back2top:

Same for exturl:

exturl: false

@stevenjoezhang stevenjoezhang changed the title Bugfix: b2t on mobile (#482), sidebar item margin and sidebar link color Bugfix: back2top button on mobile (#482), sidebar item margin and sidebar exturl link color Mar 16, 2019
@ivan-nginx
Copy link
Member

Maybe this step also needs to be automated. For example, there are 4 different components in the sidebar, each one can be enabled/disabled, then there are 2^4=16 combinations in total... need to make sure they are all suitable, and it takes time for human to test.

Nobody talk NexT is easy. ;)
Main of Vi's thinking also was: anything can be a component.
All things need to do simplier, smaller. If files can be splitted on several pieces → this also great choice of good coding.

Copy link
Member

@ivan-nginx ivan-nginx left a comment

Choose a reason for hiding this comment

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

And, yes, there is no feature, just label «CSS» and «Code style update».

@ivan-nginx ivan-nginx changed the title Bugfix: back2top button on mobile (#482), sidebar item margin and sidebar exturl link color Bugfix: back2top button on mobile, sidebar item margin and sidebar exturl link color Mar 16, 2019
@ivan-nginx
Copy link
Member

Please, no need to paste issues inside titles. They already exists in PR content and issue will auto-closed:
image

@stevenjoezhang
Copy link
Contributor Author

@ivan-nginx Thanks 👍 I learned a lot here.

@stevenjoezhang stevenjoezhang merged commit 8a9b7e9 into theme-next:master Mar 16, 2019
@stevenjoezhang stevenjoezhang deleted the b2t branch March 16, 2019 17:36
@ivan-nginx
Copy link
Member

@stevenjoezhang also, if there is label Bug Fix and there are no Breaking Changes, don't forget to add v6.x label for cherry-pick this PR into version 6 in accordance with this #567 issue.

@liolok liolok changed the title Bugfix: back2top button on mobile, sidebar item margin and sidebar exturl link color back2top button on mobile, sidebar item margin and sidebar exturl link color Mar 31, 2019
tongluyang pushed a commit to tongluyang/hexo-theme-next that referenced this pull request Nov 19, 2019
… link color (theme-next#684)

* Bugfix: show `back2top` button on mobile (theme-next#482)

* Optimize sidebar item margin

* Fix wrong `exturl` link color
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Next的Pisces主题样式在移动端似乎没有Back to Top的功能?
2 participants