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

Use window size to determine sidebar display, and unify behaviour for margin and dimmer #130

Merged
merged 2 commits into from
Nov 19, 2020

Conversation

tusooa
Copy link
Contributor

@tusooa tusooa commented Oct 22, 2020

PR Checklist

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

PR Type

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

What is the current behavior?

  1. Screen size is used to determine whether sidebar is shown on page load, instead of window size.
    This means those who have a narrow window on a wide screen will still have the sidebar shown.
    This covers page content.

  2. On "tablet" view (between "desktop" and "mobile"), the page content will be covered by the sidebar (-margin), but it is not dimmed (-dimmer).
    -margin -dimmer

    Compare:

    • "desktop" view (+margin -dimmer): +margin -dimmer

    • "mobile" view (-margin +dimmer): -margin +dimmer

Issue resolved: N/A

What is the new behavior?

  1. Window size is used to determine whether to show sidebar on page load.
  2. -margin will always go with +dimmer. On "tablet" view, it will display as -margin +dimmer (same as "mobile" view).
  • Link to demo site with this changes: N/A
  • Screenshots with this changes: See above

How to use?

N/A

@welcome
Copy link

welcome bot commented Oct 22, 2020

Thanks so much for opening your first PR here!

@CLAassistant
Copy link

CLAassistant commented Oct 22, 2020

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the CSS label Oct 22, 2020
@tusooa
Copy link
Contributor Author

tusooa commented Oct 22, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

What is the relationship between NexT and SAP SE? In the agreement it asked me to give my rights to SAP SE, but I did not see anything mentioning relationship between NexT and SAP SE on NexT websites (So I definitely will not sign it for now.). I looked up https://cla-assistant.io/ and it seems to indicate projects should provide their own agreements. Was the one for NexT set up properly according to their guidelines?

@@ -2,7 +2,7 @@
display: none;
}

+mobile() {
+tablet-mobile() {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps unifying the behavior of the tablet and desktop is a better choice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think "Tablet" has enough space for both the article and sidebar.

Copy link
Member

Choose a reason for hiding this comment

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

This problem is a bit tricky, because both 7.9-inch iPad mini and 12.9-inch iPad Pro are "Tablet"s.

I will merge this pull request and make adjustments based on subsequent user feedback. Thanks for your contribution.

@stevenjoezhang stevenjoezhang added this to the 8.1.0 milestone Nov 16, 2020
@stevenjoezhang stevenjoezhang merged commit 4581626 into next-theme:master Nov 19, 2020
@welcome
Copy link

welcome bot commented Nov 19, 2020

Congrats on merging your first pull request here! 🎉 How awesome!

moonfox pushed a commit to moonfox/hexo-theme-next that referenced this pull request Apr 5, 2021
lingyf pushed a commit to lingyf/hexo-theme-next that referenced this pull request Jan 27, 2022
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.

3 participants