Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

7623 - Subcategories - front-end styling for desktop #7712

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

fessehaye
Copy link
Contributor

@fessehaye fessehaye commented Oct 28, 2021

Closes #7623

Also includes some config changes for eslint and prettier for our node tests

Does not include mobile subcategory tasks

https://foundation-s-7620-pills-bdzrnb.herokuapp.com/en/privacynotincluded/

@fessehaye fessehaye requested a review from Pomax October 28, 2021 18:10
@mofodevops mofodevops temporarily deployed to foundation-s-7620-pills-xrnlpf October 28, 2021 18:10 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-7620-pills-xrnlpf October 28, 2021 18:20 Inactive
@fessehaye fessehaye closed this Oct 28, 2021
@fessehaye fessehaye reopened this Oct 28, 2021
@mofodevops mofodevops temporarily deployed to foundation-s-7620-pills-bdzrnb October 28, 2021 18:58 Inactive
@fessehaye fessehaye changed the title 7620 - Subcategories - include subcategory pills on the Product Page 7623 - Subcategories - include subcategory pills on the Product Page Oct 28, 2021
@mofodevops mofodevops temporarily deployed to foundation-s-7620-pills-bdzrnb November 1, 2021 18:31 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-7620-pills-bdzrnb November 1, 2021 18:32 Inactive
@mofodevops mofodevops temporarily deployed to foundation-s-7620-pills-bdzrnb November 1, 2021 18:52 Inactive
@fessehaye fessehaye requested a review from sabrinang November 1, 2021 18:56
Copy link

@sabrinang sabrinang left a comment

Choose a reason for hiding this comment

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

  • add a bit more space above creep-o-meter introduction (~32px)
    image

  • From the mockups, it looks like the heading of the main category doesn't change and is clickable (dark-blue on hover) to reset the category (like as if you clicked the category from the navigation bar). See Figma mockup
    image

  • Some subcategory pages don't show the state where there isn't a product:
    image

  • Some others have it:
    image

@Pomax Pomax changed the title 7623 - Subcategories - include subcategory pills on the Product Page 7623 - Subcategories - front-end styling for desktop Nov 1, 2021
Pomax
Pomax previously requested changes Nov 1, 2021
Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

A few notes - also it looks like seaching does not change the title on the page to "All", even though it does highlight the "all" entry in the nav bar:

image

@mofodevops mofodevops temporarily deployed to foundation-s-7620-pills-bdzrnb November 1, 2021 23:12 Inactive
@fessehaye fessehaye requested a review from sabrinang November 1, 2021 23:12
Copy link

@sabrinang sabrinang left a comment

Choose a reason for hiding this comment

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

Thanks for making the other changes! I found one more thing when I click on the other categories and click back to all ... the heading doesn't change back to all. For example when I visited pets and clicked all (should show All heading):
image

@fessehaye fessehaye requested a review from sabrinang November 1, 2021 23:27
@fessehaye
Copy link
Contributor Author

That is because the "ALL" link still points to mofo-staging.

Tagging @Pomax just to let him know but the all link does work properly on my dev env

@mofodevops mofodevops temporarily deployed to foundation-s-7620-pills-bdzrnb November 1, 2021 23:39 Inactive
@fessehaye fessehaye requested a review from Pomax November 1, 2021 23:39
Pomax
Pomax previously requested changes Nov 2, 2021
Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

one code issue, but hitting "delete" in the search does not restore the category the user was on before they started searching. Instead, we stay on "all", but with the products not sorted on creepiness even though there is no search term,

E.g. be on a category:

image

start searching

image

delete search again:

image

If we want to keep the user on "all", then that's fine, but then we'll need to make sure to at the very least re-sort the products on creepiness (low to high ) when there is no text in the search box. I.e. the same ordering as when you intentionally navigate to All:

image

Co-authored-by: Pomax <pomax@nihongoresources.com>
@mofodevops mofodevops temporarily deployed to foundation-s-7620-pills-bdzrnb November 2, 2021 00:30 Inactive
@Pomax
Copy link
Contributor

Pomax commented Nov 2, 2021

@sabrina when a user starts to search, and then deletes their search again (either though delete/backspace, or the (X) button) should we take the user back to the category they were on before they started searching, or do we keep the user on "all"?

Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

(for some reason the review had been marked as "comment" instead of "changes requested"...)

@sabrinang
Copy link

@Pomax I think it should just stay in "all" if they remove/start searching instead of going back to a previous catergory

Copy link

@sabrinang sabrinang left a comment

Choose a reason for hiding this comment

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

Approving, Simon explained that "All" does have the right behaviour but isn't showing on staging

@fessehaye fessehaye requested a review from Pomax November 2, 2021 17:01
@fessehaye fessehaye merged commit 235d26f into pni-q3-2021 Nov 2, 2021
@fessehaye fessehaye deleted the 7620-pills branch November 2, 2021 17:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants