fix(components): Display Datalist checkbox based on device type instead of breakpoint

MichaelParadis commented 2 months ago

Motivations

On touch screen devices that were wider than our --medium-screens-and-up the DataList checkbox wouldn't be displayed because the user wouldn't be able to hover over the row, this PR works to fix that.

Changes

Added

Changed

Deprecated

Removed

Fixed

components: Display DataList checkboxes on touch screen devices that are medium screen sized and up.

Security

Testing

Can be tested in storybook here: https://5607bc22.atlantis.pages.dev/iframe?args=&id=components-lists-and-tables-datalist-web--basic&viewMode=story

Can also be tested in JO, see the PR mentioning this one.

To test enable responsive mode in your browser inspector (this is wider screen with touch simulation enabled):

Verify:

When on the medium-screens-and-up size the check box appears when touch simulation is enabled
Verify the checkboxes do not appear on the medium-screens-and-up size when touch simulation is disabled
Verify smaller breakpoints work as expected when touch simulation is enabled / disabled

Wider screen with touch simulation disabled (hovering on a row):

Smaller screen with touch simulation disabled:

Smaller screen with touch simulation enabled:

Changes can be tested via Pre-release

In Atlantis we use Github's built in pull request reviews.

Random photo of Atlantis

github-actions[bot] commented 2 months ago

Published Pre-release for 02b49def1c8632c8bb70d3a03ded43f34660ddd3 with versions:

  - @jobber/components@5.29.1-JOB-97858-02b49de.6+02b49def
  - @jobber/components-native@0.71.2-JOB-97858-02b49de.20+02b49def

To install the new version(s) for Web run:

npm install @jobber/components@5.29.1-JOB-97858-02b49de.6+02b49def

To install the new version(s) for Mobile run:

npm install @jobber/components-native@0.71.2-JOB-97858-02b49de.20+02b49def

cloudflare-workers-and-pages[bot] commented 2 months ago

Deploying atlantis with Cloudflare Pages

Latest commit:	`02b49de`
Status:	✅ Deploy successful!
Preview URL:	https://a3dbb5f0.atlantis.pages.dev
Branch Preview URL:	https://job-97858-datalist-checkbox.atlantis.pages.dev

View logs

chris-at-jobber commented 2 months ago

@MichaelParadis one thought - do we actually need a breakpoint query at all? If the user can hover a narrow list, why do we need to show the checkbox permanently?

zevpermack commented 2 months ago

Thanks for taking this on!! Tested the prerelease and works to solve what we talked about on quotes and client. I don't see the checkboxes come up on jobs. I'm guessing that is because they don't have bulk actions yet?

I'm not sure I 100% follow what Chris was saying but I was wondering if (--medium-screens-and-up) might be redundant now if we want all pointer cases. I guess the case of smaller breakpoints having the checkboxes available is handled elsewhere. Happy to let your team handle the code review since I don't have a lot of context there though and I'm sure I'm missing something.

Also just curious to learn why you ended up going for fine instead of coarse for any-pointer?

MichaelParadis commented 2 months ago

@zevpermack and @chris-at-jobber

RE: The `--medium-screens-and-up` comments

I left it as before because the checkbox wouldn't display all the time on small screen sizes without hovering on desktop. I feel like this could lead to some confusion on smaller breakpoints.

Current change

Removing the medium-screens-and-up and being on a non-touchscreen device

RE: `any-pointer: fine vs any-pointer: course` I went with `fine`

The behaviour we are trying to have is have the checkbox hidden when there is a mouse and display it on hover. The media query here is for hiding the checkbox unless it is hovered so using course would reverse the behaviour (it would show all the time for mice and need to be hovered for it to display when there isn't a mouse (not possible). This is the CSS that changes the opacity when the checkbox is hovered

any-pointer mdn page

zevpermack commented 2 months ago

Loaded up Atlantis repo and confirmed all of that. LGTM, thanks @MichaelParadis

chris-at-jobber commented 2 months ago

Thanks @zevpermack ! @MichaelParadis I'm going to chat with @jlelford (the original designer who flagged this) and @diajjiao for their thoughts on the need for a media query vs cursor query.

chris-at-jobber commented 2 months ago

Bringing the outcome of the design conversation here:

If we can ensure that in cases where the cursor query might fail, we show the checkbox, then we can do away with the breakpoint check.

GetJobber / atlantis