-
-
Notifications
You must be signed in to change notification settings - Fork 729
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] Cart Dropdown #5480
[Mobile] Cart Dropdown #5480
Conversation
a828110
to
1338e7a
Compare
5a073a2
to
5ecce2a
Compare
5ecce2a
to
b5ab345
Compare
Awesome, I will review soon. Do you plan to fix any of the codeclimate issues? Like the single quotes ones? |
I fixed a number of them. I'll have another look and assess some rules. Most of these linting issues were due to moving existing CSS rules from one file into two clearer files. 🙈 |
b5ab345
to
4aac042
Compare
I've fixed some more linting issues, and disabled two rules: We use double-quoted strings all over the place, and we've decided in the rest of the codebase to prefer double-quoted strings (and disable the same rule in rubocop). This rule is only triggered because we extracted breakpoint declarations into a mixin, which I think is an improvement. It would be a total mess if we actually tried to follow this rule everywhere we use breakpoints... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work Matt. A lot of work in this PR 💪
Just one major point about Cart.finalized_line_items.
please don't force-push 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impecável, my new approval word :-)
%img{'ng-src' => '{{ line_item.variant.thumb_url }}'} | ||
%td | ||
%span {{ line_item.variant.extended_name }} | ||
%br |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't we use a block element instead of a span
so there's no need for a br
? the need for it clearly shows the span
doesn't fit the needs.
@sauloperez I removed some of the indentation changes. It's probably best not to touch any of that CSS in this PR as I think Maikel will be changing it a lot in other open PRs, and it wasn't strictly necessary or relevant here. All the above comments are now outdated / not relevant. |
The CTA breaks the layout when the new cart sidebar is open. I can't see a nice way to keep it at the top without making a mess.
This behavior has been removed
This was mistakenly changed to use a max-width breakpoint in a previous commit.
This feature has been removed
Fixes a minor visual bug
…n /app/assets/images folder This seems to be needed for Rails 4
7e4b8e0
to
1511551
Compare
Rebased to resolve conflicts. |
Conflicts don't seem to be resolved, semaphore still failed. Deploying anyway. |
Ok, so, the take me shopping button is now displaying when you've visited as shop and then go back to the /shops page. However, it's also showing up on desktop/tablet, which it isn't supposed to do. @yukoosawa this is something to consider when you do your final design sweep on the cart experience. @Matt-Yorkley, go ahead and resolve the conflicts and then merge. |
Shows "Take me shopping!" button on /shops page if a shop is selected
1511551
to
b189d06
Compare
What? Why?
Closes #4704
Closes #2718
Closes #4705
Closes #2706
What should we test?
Everything in the cart dropdown epic.
Release notes
Updated UX on cart dropdown for all devices.