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

feat: Less obtrusive clock close button #4793

Merged
merged 3 commits into from
Jul 26, 2019
Merged

Conversation

sulkaharo
Copy link
Member

@sulkaharo sulkaharo commented Jul 24, 2019

  • Close button color is much dimmer and changes color appropriately in the color face
  • HIDE_CLOCK_CLOSEBUTTON setting to hide the button altogether (set to TRUE or ON to hide buttons altogether)
  • Fix CSS for toolbar positioning in the main view
  • Fix documentation on the clock URLs and add mention of the setting

…n the color view

* HIDE_CLOCK_CLOSEBUTTON setting to hide the button altogether (set to TRUE or ON to hide buttons altogether)
* Fix CSS for toolbar positioning in the main view
* Fix documentation on the clock URLs and add mention of the setting
@PieterGit
Copy link
Contributor

PieterGit commented Jul 24, 2019

Hate to be nitty gritty. We don't have HIDE_* environment variables yet, but we do have SHOW_*.
Shouldn't we use SHOW_CLOCK_CLOSEBUTTON with a default of true to be consistent?

@PieterGit PieterGit added this to the 0.13.0 milestone Jul 24, 2019
@PieterGit PieterGit changed the title Less obtrusive clock close button feat: Less obtrusive clock close button Jul 24, 2019
@sulkaharo
Copy link
Member Author

@PieterGit changed

@nightscout nightscout deleted a comment Jul 24, 2019
@unsoluble
Copy link
Contributor

Still need to add for the record that the buttons on the clock views aren’t actually necessary in any situation, but I guess I’ve lost this one. :)

@unsoluble
Copy link
Contributor

Also though: This one should be pushed out now, not left for 0.13 — the toolbar spacing offset in the main view actually breaks functionality.

@PieterGit PieterGit modified the milestones: 0.13.0, 0.12.3 Jul 24, 2019
@PieterGit
Copy link
Contributor

@unsoluble can you confirm that this PR works as expected (although you had like a different default). Depending on users knowing the Apple gestures is not a good userinterface thing. Nightscout should be usable withouth Apple features. I noted clock stuff still is not token-based proof, so I can't test it.

@unsoluble
Copy link
Contributor

unsoluble commented Jul 24, 2019

Users don't need to know Apple gestures to use the site in nearly every single use case. In the one specific situation where an iOS 12 user has pinned the main site to the home screen, and then ventures to a clock view from there, a gesture is required to get back to the main view. Honestly I'd wager this navigation path essentially never happens — adding a UI element with a controlling variable for a 0.00001% edge case like this seems like brutal overkill to me.

(I wrote the clock-color view; I know what its intended purpose is, and adding visual clutter by default is not good.)

If we absolutely must have this extra UI control in there as an option, I strongly suggest we default it to false, as people have been using these views for years without a button, and having one suddenly appear is bad. We can make it clear in the docs how to enable the button for those who for some reason feel they need it.

Will test when I get a chance, but probably can't be till later tonight.

@unsoluble
Copy link
Contributor

(#4788 should be included in this too.)

@nightscout nightscout deleted a comment Jul 24, 2019
@unsoluble
Copy link
Contributor

Tested this branch — LGTM. If we default SHOW_CLOCK_CLOSEBUTTON to false, we can call it a truce. :)

@sulkaharo sulkaharo merged commit 71981de into dev Jul 26, 2019
@unsoluble
Copy link
Contributor

We’re going with a false default though, right?

@sulkaharo sulkaharo deleted the less_obtrusive_clock_close branch July 29, 2019 14:16
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