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

[mobile ux] Merge search and filter epic to master #5330

Merged
merged 49 commits into from
May 13, 2020
Merged

Conversation

Matt-Yorkley
Copy link
Contributor

@Matt-Yorkley Matt-Yorkley commented Apr 29, 2020

What? Why?

Covers epic #4601

This PR is merging the mobile-ux-filters feature branch into master.

What should we test?

All six issues in #4601

Release notes

Major updates to search and filter UX in shops, on all devices

@Matt-Yorkley Matt-Yorkley self-assigned this Apr 29, 2020
@Matt-Yorkley Matt-Yorkley marked this pull request as ready for review April 30, 2020 06:30
@Matt-Yorkley
Copy link
Contributor Author

Rebased to resolve conflicts...

@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented May 1, 2020

I manually approved some CSS linting issues with CodeClimate here. It looks like CodeClimate only runs on the master branch, so it was not linting any of the previous PRs submitted to the feature branch before now. The 3 categories of issues were:

  • "prefer single quotes". We use double quotes in almost all places, eg @include "mixins" and we've decided elsewhere in the codebase to prefer double quotes.
  • "nesting too deep (4 / 3) ". I think this will require a huge refactor in some cases. If we want to do this we should probably wait until after mobile UX is finished?
  • the third category related to the recent refactoring of our breakpoint definitions into variables. Now that it's in an @include statement, CodeClimate complains that @include statements should come before properties in the ordering, and wants every single use of breakpoints to be moved around. I don't think this is a good idea in this specific case, it'll make things much less clear.

Maybe we can run an automated CSS linting and refactoring tool on all of it after we've finished mobile? There are several very good tools that can do that (like rubocop's autocorrect, but for CSS).

@Matt-Yorkley Matt-Yorkley changed the title [Mobile] Search and filter updates [Mobile] Merge search and filter epic to master May 1, 2020
@daniellemoorhead daniellemoorhead changed the title [Mobile] Merge search and filter epic to master [Mobile ux] Merge search and filter epic to master May 2, 2020
@daniellemoorhead daniellemoorhead changed the title [Mobile ux] Merge search and filter epic to master [mobile ux] Merge search and filter epic to master May 2, 2020
@daniellemoorhead daniellemoorhead added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels May 4, 2020
@daniellemoorhead
Copy link
Contributor

daniellemoorhead commented May 7, 2020

There are conflicts @Matt-Yorkley - if you could resolve these and then stage this on AU today then @yukoosawa can have a look at it during her time and then I can take a look on Friday. 🙏

Yuko, Baw Baw is up on AU staging so you can use them as your test shop.

@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented May 7, 2020

@daniellemoorhead @yukoosawa merge conflicts resolved. I'll stage it now, It'll be ready in 15 minutes or so 👍

@daniellemoorhead daniellemoorhead added the pr-staged-au staging.openfoodnetwork.org.au label May 7, 2020
@daniellemoorhead
Copy link
Contributor

Dammit, this has conflicts again @Matt-Yorkley 😞

@Matt-Yorkley
Copy link
Contributor Author

Yeah... with multiple devs all touching the same CSS files in different PRs, it'll be a carnival of merge conflicts.

I'll have a look at rebasing it now 👍

This required a lot of refactoring, as the search needed to be inside both the form element and the Angular ProductsCtrl element, but to get a full-width row for the searchbar it needed to be outside of the 12 column layout of the other shop page elements...
Also includes a minor refactor to resolve an issue with animation timings. Angular was not adding the "shown" class to the different elements at the same time in the digest cycle, and it looked a bit shaky.
In theory this should improve some of the custom-keyboard functionality added by mobiles, but the implementations will be vary...
Hopefully this will work on iPhones as well...?
Previously for example with "Organic" property and "Fruit" and "Nuts" categories it rendered as: "Fruit or Nuts Organic" instead of: "Fruit or Nuts or Organic"
This doesn't fit with the new syntax structure
@Matt-Yorkley
Copy link
Contributor Author

Okay @daniellemoorhead the conflicts are resolved 👍

@daniellemoorhead
Copy link
Contributor

Second round of testing (full notes in testing document):

For #4603:

  • All failing tests in notes are from @yukoosawa so I'll leave her to test those and respond.

For #4604:

  • Help words in the search field still cut off, they need to be shortened to just say "Search..." like in the new design.
  • The filter button doesn't change colour on hover (see notes for what it should do).
  • Search bar now doesn't span the window (see screenshots in notes).
  • Plus failing notes from @yukoosawa to be tested.

For #4602

  • Hitting 'search' button on keyboard doesn't shut the keyboard.
  • Two cancel buttons are displaying on desktop (see screenshots in notes).
  • Plus failing notes from @yukoosawa to be tested.

For #4605:

  • All failing tests in notes are from @yukoosawa so I'll leave her to test those and respond.

For #4669:

  • All failing tests in notes are from @yukoosawa so I'll leave her to test those and respond.

@Matt-Yorkley it might be worth either waiting till @yukoosawa has done her second round of testing, because she's likely to have some things that also still require tweaks (I assume you checked the testing notes and worked through her comments?).

@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented May 12, 2020

Ah, since other Mobile PRs have been merged and the branch has been rebased, things are suddenly broken. 😭

This will probably keep happening until we have only one dev on Mobile at a time...

@Matt-Yorkley
Copy link
Contributor Author

Okay @daniellemoorhead and @yukoosawa I've fixed things up a bit. Notes:

  • Help words in the search field still cut off, they need to be shortened to just say "Search..." like in the new design.

This is updated, it's just that the new translations have not been made and will not be in staging, so you won't see the changes yet. You can actually hack this by going to this link first to see the site using the base language file: https://staging.openfoodnetwork.org.au/?locale=en

  • The filter button doesn't change colour on hover (see notes for what it should do).

It does, I promise!

ch-ch-ch-ch-changeeees

  • Search bar now doesn't span the window (see screenshots in notes).

Fixed 👍

  • Hitting 'search' button on keyboard doesn't shut the keyboard.

Not fixed. I have no idea how to hack the UX on the built-in iPhone keyboard, and I don't have an iPhone. Can we move this to another issue and have someone else take a look at it?

  • Two cancel buttons are displaying on desktop (see screenshots in notes).

Fixed 👍

  • I assume you checked the testing notes and worked through her comments?

Yes, I think it's all covered.

@daniellemoorhead
Copy link
Contributor

daniellemoorhead commented May 12, 2020 via email

@yukoosawa
Copy link

Second round of testing!


As this is my [epic] round last chance review, I have one last UI tweak:

  • Reduce top/bottom padding around search field + filter button [mobile & tablet only]. Should be 0.5em/8px so it matches the OC selector bar above it.

#4603

#4604:

#4602

  • Hitting 'search' button on keyboard doesn't shut the keyboard.

@daniellemoorhead Do you want to make a call on this based on Matt's comments re: not knowing how to fix this? I've tested on iPhone 6S (running older iOS 11), iPad mini 3 (running older iOS 12), and iPhone 11 Pro (running iOS 13) and for all 3 devices tapping the 'search' button doesn't close the keyboard.

#4605:

#4669:

@daniellemoorhead
Copy link
Contributor

  • Hitting 'search' button on keyboard doesn't shut the keyboard.
    Not fixed. I have no idea how to hack the UX on the built-in iPhone keyboard, and I don't have an iPhone. Can we move this to another issue and have someone else take a look at it?

I'll write another card for this, let someone else take it (maybe @mkllnk who has an iphone, or @luisramos0 also has one).

@Matt-Yorkley -> everything else is OH EM GEE GOOD TO GO!

@daniellemoorhead daniellemoorhead removed the pr-staged-au staging.openfoodnetwork.org.au label May 13, 2020
@luisramos0 luisramos0 merged commit ef2d7f9 into master May 13, 2020
@luisramos0 luisramos0 deleted the mobile-ux-filters branch May 13, 2020 07:59
@luisramos0
Copy link
Contributor

👏 👏 👏

@luisramos0 luisramos0 mentioned this pull request May 18, 2020
7 tasks
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.

5 participants