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: shop tabs #4655

Merged
merged 19 commits into from
Jan 20, 2020
Merged

Conversation

Matt-Yorkley
Copy link
Contributor

@Matt-Yorkley Matt-Yorkley commented Jan 7, 2020

What? Why?

Closes #4568

  • Splits the product list and shop "messages" into separate tabs, instead of showing the products under every tab
  • Adjusts the layout and CSS for mobile and desktop
  • Improves show/hide logic of shop tabs, eg the groups tab now does not display if the enterprise is not part of a group
  • Doesn't include all the restyling / colour scheme changes etc, but looks good enough to merge

What should we test?

Functionality of the tabs and tab content in shopfronts. From the issue description:

  • Check that clicking on a tab link loads a new page with its own URL.
  • Check that the groups tab only shows if the shop is part of a group.
  • Check that the home tab only shows when the shop has a shopfront message.
  • Check that when there is no shopfront message, the default tab to open is shop.
  • Check that when there is a shopfront message, the default tab to open is home.
  • Check that the underline displays on the tab that is open, for every tab.
  • User should be able to copy the link for a specific tab (eg "About") and the link should correctly take you to that section

Release notes

Updated shopfront tabs and content display. Product list is now a separate tab.

Changelog Category: Changed

@Matt-Yorkley Matt-Yorkley self-assigned this Jan 7, 2020
@Matt-Yorkley
Copy link
Contributor Author

I'm really happy with this! :)

On a side note, can we disable or ignore some of these SCSS linting rules whilst doing the mobile work? I touched / indented some existing CSS and it's demanding I rearrange all the properties in alphabetical order and giving some obtuse warnings about "depth of applicability" and nesting being "4" levels instead of "3".

@sigmundpetersen
Copy link
Contributor

I wonder if hiding the groups tab functionality is also part of #4647?

@luisramos0
Copy link
Contributor

Nice!!! I'll review soon.

I think the css rules are ok, nesting should be limited, right? I think we can ignore them if you want 👍

@luisramos0
Copy link
Contributor

can you share some screenshots @Matt-Yorkley ?

@Matt-Yorkley Matt-Yorkley force-pushed the mobile-shoptabs branch 2 times, most recently from b644d4a to b9985ac Compare January 8, 2020 13:17
@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented Jan 8, 2020

SHOP TAB, DESKTOP:

Screenshot from 2020-01-08 14-20-01

PRODUCERS TAB, DESKTOP:

Screenshot from 2020-01-08 14-20-12

SHOP TAB AND NAVIGATION, MOBILE:

Screenshot from 2020-01-08 14-20-52

ABOUT TAB, TABLET:

about_tab

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Great work, Matt!

When creating the task, we were not thinking of Angular tabs. We thought that we would move them to their own pages. That could save some code (product reset) but your solution is much faster to browse. 👍

I left two comments which address very small style issues but I think that they are worth a couple of changes.

app/helpers/shop_helper.rb Outdated Show resolved Hide resolved
app/helpers/shop_helper.rb Outdated Show resolved Hide resolved
@Matt-Yorkley
Copy link
Contributor Author

Thanks for the review @mkllnk, the shop helper methods and that view are much nicer now 😄

Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

looking good! I thought the tab HOME would be deleted...

@Matt-Yorkley
Copy link
Contributor Author

Matt-Yorkley commented Jan 12, 2020

I thought the tab HOME would be deleted...

The home tab is new, its where shopfront messages go now. Some of the UX discussion is here: #4567 (comment)

Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

awesome! I left some minor comments.

I think you need to add to tests the verification of these tabs in other pages other than shopfront page: cart, checkout and on the page where you view a completed order (account) if the current shop is the shop where the order was placed (see app/views/spree/orders/show.html.haml)

app/assets/stylesheets/darkswarm/shop_tabs.css.scss Outdated Show resolved Hide resolved
app/helpers/shop_helper.rb Outdated Show resolved Hide resolved
app/views/shopping_shared/tabs/_contact.html.haml Outdated Show resolved Hide resolved
Angular controller data was being partially preserved when switching back and forth between tab templates, causing the ProductsCtrl to hold duplicate datasets when it is re-initialized after going from the shop tab to another tab, then back again.
@daniellemoorhead daniellemoorhead self-assigned this Jan 16, 2020
@daniellemoorhead
Copy link
Contributor

When creating the task, we were not thinking of Angular tabs. We thought that we would move them to their own pages. That could save some code (product reset) but your solution is much faster to browse. 👍

Hey @Matt-Yorkley and @mklink will the Angular tabs allow for shops to link directly to them? That was the purpose of having separate pages, I'll check if I can answer this question myself when I'm testing...let's see!

@daniellemoorhead
Copy link
Contributor

@Matt-Yorkley testing notes for you (awesome job btw 🎉 )

  1. The tabs are links, so completely disregard that last comment I made above 😄

  2. The new home tab needs the shopfront message to be on the page itself, it shouldn't be within a closable green box anymore. Would this be something that'll come as part of the restyling or should it be done as part of this piece of work?

Screen Shot 2020-01-19 at 7 47 09 pm

  1. Is there a reason you have the mobile tabs in a different design look than tablet and desktop? Given the new design has them all with a line underneath it would make sense to have these tabs using the same design so that they're easily restyled, is that a fair assumption? If this PR is going to be merged and released without the restyling then the design will need to be changed to be underlined.

Tablet with the underlined tab is correct styling
Screen Shot 2020-01-19 at 7 38 06 pm

  1. On the about tab the content cuts off so that you can't see some of the URL. When there's spaces and copy it's ok, but it needs to deal with URLs better. Would this be a separate bug to be logged or is it an easy fix on here?

IMG_A353B6DE072D-1

@Matt-Yorkley
Copy link
Contributor Author

@daniellemoorhead

  1. 👌
  2. Yeah, I wasn't sure about the design for those shop messages (there wasn't anything in Zeplin). It would be great to get rid of the green boxes!
  3. Actually I didn't change the styling on the mobile shop tabs, I just resized some bits. So it's part of the old default CSS? Should be easy to update.
  4. That's weird that the link goes outside of the content area. I'll take a look.

It probably makes sense to include those 3 small bits in this PR. 👍

@Matt-Yorkley
Copy link
Contributor Author

Okay, the green boxes on shopfront messages are gone, and the styling of the buttons on mobile now looks the same as on desktop. 👍

I couldn't replicate that issue with the long link going slightly off the page. It always wraps inside the box on my machine, even in mobile / tablet layout. It could be a browser-specific issue, in which case it probably wants looking at in another PR.

@daniellemoorhead
Copy link
Contributor

Awesome news @Matt-Yorkley! I'll retest today or tomorrow.

I couldn't replicate that issue with the long link going slightly off the page. It always wraps inside the box on my machine, even in mobile / tablet layout. It could be a browser-specific issue, in which case it probably wants looking at in another PR.

I'll see if I can get it to replicate on other phones (will try new iPhones and Androids here in the office today). Stay tuned.

@daniellemoorhead
Copy link
Contributor

Testing round 2:

  • green box gone (hoorah!)
  • tabs have same stying as on tablet/desktop

This is good to go live!

Ping @kirstenalarsen who is going to be very happy to see movement on mobile stuff again :-)

@mkllnk
Copy link
Member

mkllnk commented Jan 21, 2020

Wooohooo!

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.

[mobile ux] Move shop content to tabs with URLs
5 participants