Fix search sizing on mobile (explore navbar overflow) #1760

kaixin-hc · 2022-02-12T09:51:29Z

What is the purpose of this pull request?
Fixes #1429

Also related: #1699

Overview of changes:

Set a minimum width for the scrollbar to be 8em. I tested this on the different device sizes of chrome and I think it looks fine on all; should guarantee at least least the whole placeholder attribute 'search' to be visible on the smallest screen sizes.
Make the section with items in the navigation bar scrollable in the case of overflow

Anything you'd like to highlight / discuss:

@jonahtanjz since this modifies your previous changes for mobile, I'd like your opinion!
Hiding the scrollbar in the navigation bar looks nice, but I'm not sure if it's very accessible.
Used em instead of vw for accessibility reasons(as it scales if the user zooms the screen)

Testing instructions:
Add lots of elements to the navigation bar to see it scroll + use google chrome mobile.

Proposed commit message: (wrap lines at 72 characters)
Make navbar scrollable & optimise search input size

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

tlylt

Hiding the scrollbar in the navigation bar looks nice, but I'm not sure if it's very accessible.

I agree with you that it looks nice without the scrollbar but I feel that indeed there might be valid concerns about accessibility.

In an extreme case where the navbar has quite a few items and some of them are nicely hidden behind the search box, it might be frustrating for site visitors who then become unaware & unable to find the target nav item.

I think perhaps one way is to explore creating custom css scrollbar which may be more visually appealing.
Or, perhaps we can consider something like what the Github extension that I am using is doing?

kaixin-hc · 2022-02-12T10:57:33Z

What github extension are you using? I think putting a dropdown for excess navbar elements might be a better solution than a custom scrollbar.

For reference for what it looks like with a scrollbar:

Honestly, I don't think it's too bad either, but it's also one of those things that doesn't trigger unless you actually try to scroll on the screen (and I'm not sure if it would work with windows computers, since I scroll using an apple mouse and I'm not sure if the scroll bar would activate on windows since it only shows up when I try to scroll). So it wouldn't help people who didn't know the navbar was intended to be scrollable.

tlylt · 2022-02-12T11:21:34Z

What github extension are you using?

https://github.com/refined-github/refined-github

jonahtanjz · 2022-02-13T07:02:02Z

Hiding the scrollbar in the navigation bar looks nice, but I'm not sure if it's very accessible.

The issue with using the native scrollbar is that it can differ depending on OS and browser. For example this is how it looks on windows 10 Chrome

In this case I would agree with @tlylt to create a custom scrollbar for desktop to keep the design consistent.

Or, perhaps we can consider something like what the Github extension that I am using is doing?

I think putting a dropdown for excess navbar elements might be a better solution than a custom scrollbar.

I think this is a good idea. We can follow how Github is doing it 👍

ong6 · 2022-02-13T07:25:13Z

Just throwing my opinion out here, but I think a native scrollbar is not the best solution, a dropdown like @tlylt and @jonahtanjz
suggested is better. Especially when users want to see all the options in the navbar.

kaixin-hc · 2022-02-13T12:10:10Z

Hmm, I'm also leaning towards the dropdown (which isn't how github does it but an extension) as opposed to the custom scrollbar. Considering this, I'll edit the description to just fixing the iPad issue? To keep changes separate, would it be possible to merge this first to fix the bug and then I make a separate PR for the dropdown?

ong6 · 2022-02-13T12:35:49Z

Hmm, I'm also leaning towards the dropdown (which isn't how github does it but an extension) as opposed to the custom scrollbar. Considering this, I'll edit the description to just fixing the iPad issue? To keep changes separate, would it be possible to merge this first to fix the bug and then I make a separate PR for the dropdown?

Yep, that sounds good.

jonahtanjz

I'll edit the description to just fixing the iPad issue? To keep changes separate, would it be possible to merge this first to fix the bug and then I make a separate PR for the dropdown?

Alright sure. Just one suggestion for the implementation of the searchbar.

jonahtanjz · 2022-02-13T13:24:45Z

packages/vue-components/src/Searchbar.vue

+<style scoped>
+    .form-control {
+        min-width: 8em;
+    }
+</style>


~~Can shift the rest of the css rules below to this scoped style as well~~

kaixin-hc · 2022-02-13T16:48:15Z

I'm not sure if we should move the CSS rules into the scoped style, due to this note on the vue documentation:

Scoped styles do not eliminate the need for classes. Due to the way browsers render various CSS selectors, p { color: red } will be many times slower when scoped (i.e. when combined with an attribute selector). If you use classes or ids instead, such as in .example { color: red }, then you virtually eliminate that performance hit.

(https://vue-loader.vuejs.org/guide/scoped-css.html#also-keep-in-mind)

Any thoughts?

jonahtanjz · 2022-02-14T08:27:51Z

Any thoughts?

#1768

jonahtanjz

@kaixin-hc, I think we can merge this first while waiting for the discussion on scoped styles.

One last thing, I noticed that by using overflow-x: scroll on the default navbar, it causes the dropdown to break.

Landing.Page.-.Google.Chrome.2022-02-15.17-18-12.mp4

It may take quite a bit of work to resolve this. Since we are intending to remove scroll for ipad/desktop and use dropdown for overflow, there is not much value in resolving this. We can limit this PR to just the searchbar and remove the changes made for the scrolling :)

This reverts commit 995af3f.

kaixin-hc · 2022-02-23T15:04:30Z

Sorry for the delay @jonahtanjz ! I've reverted the changes.

I was curious about the scrollbar issue, so I did a little digging for my own learning. After a few stack overflow posts and reading, I found this article: https://css-tricks.com/popping-hidden-overflow/ that explains that setting overflow-x sets overflow-y as well, and it's just not possible to have a dropdown peep out of a scrollable overflow without setting positions relatively. This discussion with the offered solution was also interesting. Not sure how to implement it cleanly in a vue component, but it is a non-issue.

I think it also shows the need for #1714

tlylt · 2022-02-23T15:35:37Z

@all-contributors please add @kaixin-hc for code

allcontributors · 2022-02-23T15:35:45Z

@tlylt

I've put up a pull request to add @kaixin-hc! 🎉

jonahtanjz · 2022-02-24T03:29:46Z

packages/vue-components/src/Navbar.vue

@@ -306,7 +306,6 @@ export default {
        .navbar-default {
            display: block;
            margin-top: 0.3125rem;
-            width: 100%;


@kaixin-hc I believe this is still needed so that the default-navbar will always go to the next line on mobile :)

Tried with 2 items in the default-navbar with window width of 700px:

Good catch! Though I've enclosed a picture for further reference, as I think that it looks quite strange with just two items. After some consideration I think it's still alright, since I expect this would be an edge case and it doesn't seem to have any usability issues. An alternative work around might be triggering the "go to next line" when the screen size is smaller or when the nav-bar default has a certain number of items, but I think that's more trouble than it's worth

kaixin-hc added 2 commits February 12, 2022 17:10

Scrollable navbar, larger searchbar

c18cae1

Change from vw to em for accessibility

f21f906

tlylt reviewed Feb 12, 2022

View reviewed changes

Revert accidental CSS change

3954ffe

jonahtanjz reviewed Feb 13, 2022

View reviewed changes

jonahtanjz reviewed Feb 15, 2022

View reviewed changes

kaixin-hc added 5 commits February 23, 2022 22:42

revert changes that lead to scrollbar bug

995af3f

Merge branch 'master' into dynamic-mobile

eacadde

Revert "revert changes that lead to scrollbar bug"

2757fb2

This reverts commit 995af3f.

Revert scrolling based changes

64db85d

chore: remove whitespace

d0125df

allcontributors bot mentioned this pull request Feb 23, 2022

docs: add kaixin-hc as a contributor for code #1788

Merged

jonahtanjz reviewed Feb 24, 2022

View reviewed changes

Revert width 100% to ensure it goes to next line

cee72d3

jonahtanjz approved these changes Feb 28, 2022

View reviewed changes

jonahtanjz added this to the 4.0 milestone Feb 28, 2022

jonahtanjz merged commit 0e775d5 into MarkBind:master Feb 28, 2022

kaixin-hc changed the title ~~Fix navbar overflow on mobile~~ Fix search sizing on mobile (explore navbar overflow) Mar 18, 2022

jonahtanjz mentioned this pull request Aug 14, 2022

Fix searchbar overflowing for mobile #2009

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix search sizing on mobile (explore navbar overflow) #1760

Fix search sizing on mobile (explore navbar overflow) #1760

kaixin-hc commented Feb 12, 2022 •

edited

Loading

tlylt left a comment •

edited

Loading

kaixin-hc commented Feb 12, 2022

tlylt commented Feb 12, 2022

jonahtanjz commented Feb 13, 2022 •

edited

Loading

ong6 commented Feb 13, 2022

kaixin-hc commented Feb 13, 2022

ong6 commented Feb 13, 2022

jonahtanjz left a comment

jonahtanjz Feb 13, 2022 •

edited

Loading

kaixin-hc commented Feb 13, 2022 •

edited

Loading

jonahtanjz commented Feb 14, 2022

jonahtanjz left a comment •

edited

Loading

kaixin-hc commented Feb 23, 2022 •

edited

Loading

tlylt commented Feb 23, 2022

allcontributors bot commented Feb 23, 2022

jonahtanjz Feb 24, 2022 •

edited

Loading

kaixin-hc Feb 28, 2022

Fix search sizing on mobile (explore navbar overflow) #1760

Fix search sizing on mobile (explore navbar overflow) #1760

Conversation

kaixin-hc commented Feb 12, 2022 • edited Loading

tlylt left a comment • edited Loading

Choose a reason for hiding this comment

kaixin-hc commented Feb 12, 2022

tlylt commented Feb 12, 2022

jonahtanjz commented Feb 13, 2022 • edited Loading

ong6 commented Feb 13, 2022

kaixin-hc commented Feb 13, 2022

ong6 commented Feb 13, 2022

jonahtanjz left a comment

Choose a reason for hiding this comment

jonahtanjz Feb 13, 2022 • edited Loading

Choose a reason for hiding this comment

kaixin-hc commented Feb 13, 2022 • edited Loading

jonahtanjz commented Feb 14, 2022

jonahtanjz left a comment • edited Loading

Choose a reason for hiding this comment

kaixin-hc commented Feb 23, 2022 • edited Loading

tlylt commented Feb 23, 2022

allcontributors bot commented Feb 23, 2022

jonahtanjz Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

kaixin-hc Feb 28, 2022

Choose a reason for hiding this comment

kaixin-hc commented Feb 12, 2022 •

edited

Loading

tlylt left a comment •

edited

Loading

jonahtanjz commented Feb 13, 2022 •

edited

Loading

jonahtanjz Feb 13, 2022 •

edited

Loading

kaixin-hc commented Feb 13, 2022 •

edited

Loading

jonahtanjz left a comment •

edited

Loading

kaixin-hc commented Feb 23, 2022 •

edited

Loading

jonahtanjz Feb 24, 2022 •

edited

Loading