Skip to content

Conversation

@zerosonesfun
Copy link
Contributor

@zerosonesfun zerosonesfun commented Jan 23, 2021

Adds rule for devices without hover capabilities which hides discussion item toggle button. This prevents having to tap twice to open the discussion. Also, the toggle button is not used on mobile devices and so it should be completely hidden. Additional nested rules could be added here in the future if needed.

Fixes #1913

Changes proposed in this pull request:
Adds rule for devices without hover capabilities which hides discussion item toggle button. This prevents having to tap twice to open the discussion. Also, the toggle button is not used on mobile devices and so it should be completely hidden. Additional nested rules could be added here in the future if needed. See issue: #1913

Fix uses a relatively new hover CSS media feature which can be used to test whether the user’s primary input mechanism can hover over elements. It is compatible will all major/modern browsers. No Internet Explorer support but on August 17th, 2021, Internet Explorer 11 will no longer be supported by Microsoft. All IE versions are on their way out. Also, if someone visits a forum with this one browser, the worst that would happen is they would have to double tap like before because the browser wouldn’t recognize the rule.

Reviewers should focus on:
If reviewer does not have an iPhone or iPad, reviewer could at least confirm that this fix does not affect other devices. Toggle button should still work on desktop. If reviewer has an iPhone or iPad they will be able to open a discussion with one tap.

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Adds rule for devices without hover capabilities which hides discussion item toggle button. This prevents having to tap twice to open the discussion.
@SychO9 SychO9 self-requested a review January 23, 2021 08:47
@askvortsov1
Copy link
Member

When I tested this locally, it removed it on desktop as well for me. Same with pointer: none. Not sure why though?

@zerosonesfun
Copy link
Contributor Author

zerosonesfun commented Jan 24, 2021

When I tested this locally, it removed it on desktop as well for me. Same with pointer: none. Not sure why though?

Does your Desktop have any touch or touch-like features? It looks like I should have used “any-hover” instead of “hover.” Any hover would say, “if this device has any normal hover abilities at all, don’t hide the toggle button.

https://developer.mozilla.org/en-US/docs/Web/CSS/@media/any-hover

I’ll update the pull request.

For touch devices, switching discussion list item media query “hover” to “hover-any.”

Adds rule for devices without hover capabilities which hides discussion item toggle button. This prevents having to tap twice to open the discussion.
Adds rule for devices without hover capabilities which hides discussion item toggle button. This prevents having to tap twice to open the discussion.

This is different from the original pull request. Originally “hover” was used. Now we instead use “any-hover.”
@askvortsov1
Copy link
Member

Maybe it's just my computer being weird? Either way, it still considers my computer an any-hover: none device. Very odd...

@zerosonesfun
Copy link
Contributor Author

Maybe it's just my computer being weird? Either way, it still considers my computer an any-hover: none device. Very odd...

Weird. But you can see that the any-hover rule is being applied? Your computer wishes it was a touch screen. It’s got touch screen envy.

I was trying to avoid this fix relying on screen width. Because there’s so many different sizes of tablets.

@zerosonesfun
Copy link
Contributor Author

There are other options to try as well:

https://css-tricks.com/touch-devices-not-judged-size/

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

I have confirmed that the issues I experienced are due to an issue with my Wayland setup (and even then, only occured on chrome). I'm in favor of merging this.

@zerosonesfun The issue also mentioned some problems with notifications; would you be able to include a fix for that here as well?

If touch device, hide notification action button so that you do not have to double tap.
@zerosonesfun
Copy link
Contributor Author

zerosonesfun commented Feb 4, 2021

I have confirmed that the issues I experienced are due to an issue with my Wayland setup (and even then, only occured on chrome). I'm in favor of merging this.

@zerosonesfun The issue also mentioned some problems with notifications; would you be able to include a fix for that here as well?

I started to make a change for the notifications but then I removed the change and put the file back to the way it was because I realized that notifications probably should stay as-is for now. The reason is, the first time you tap it makes a little “mark as read” check mark appear. Then you can tap the check mark to mark the notification as read without having to visit the discussion if you don’t want to. So, although it’s a little unintuitive, I don’t think the notification double tap on mobile is an “issue” per se. It’s kind of a feature so that even on mobile you have a way to individually mark notifications as read without having to visit the discussion.

What should be done down the road (I don’t know how to do it) is the notifications list should have a swipe to mark as read feature just like you can swipe discussion list items on mobile... on mobile it should be, swipe to mark as read, tap to go to the discussion.

My opinion is, let’s stick with the discussion list item fix only for now, someone could revisit notifications on mobile later.

(And I didn’t mean to request your review again. I accidentally clicked that. 😬)

Comment on lines +113 to +116
@media (any-hover: none) {
.DiscussionListItem .Dropdown-toggle.Button {
display: none; }
}
Copy link
Member

Choose a reason for hiding this comment

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

could you fix the formatting here please ? also can we use .DiscussionListItem-controls > .Dropdown-toggle instead ? it's more descriptive and doesn't have to use overspecification.

@media (any-hover: none) { 
  .DiscussionListItem-controls > .Dropdown-toggle { 
    display: none;
  } 
}

font-weight: bold;
text-transform: uppercase;
}

Copy link
Member

Choose a reason for hiding this comment

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

you removed a line by mistake

@zerosonesfun
Copy link
Contributor Author

Easier to re-do.

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.

Touch devices of certain widths forced to double tap to open discussion

3 participants