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.

Sign up for GitHub

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

Jump to bottom

Allow to change the cart trigger color #2220

Merged
merged 5 commits into from
Mar 19, 2020
Merged

Allow to change the cart trigger color #2220

merged 5 commits into from
Mar 19, 2020

Conversation

larsroettig
Copy link
Member

@larsroettig larsroettig commented Mar 5, 2020 •
edited by tjwiebell

Description

Add property to allow to change checkout trigger icon color.

Related Issue

  • [PWA-445] Make CartTrigger style extensible

Acceptance

Verification Stakeholders

Specification

Verification Steps

  • It possible to change the color from the icon

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Mar 5, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Mar 5, 2020 •
edited

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-445.

Generated by 🚫 dangerJS against 27ebf43

@larsroettig larsroettig requested review from supernova-at, sirugh and tjwiebell and removed request for supernova-at and sirugh March 10, 2020 15:55
@tjwiebell
Copy link
Contributor

Hey @larsroettig, PRs w/o issues fall outside of our normal process, sorry we missed this. Looping in @awilcoxa so we can get this added to our internal queue and reviewed.

@larsroettig
Copy link
Member Author

@tjwiebell no problem maybe I need also to create an issue. Because it was only in slack requested

@tjwiebell tjwiebell added this to In Review in Community Backlog Mar 12, 2020
@tjwiebell tjwiebell self-assigned this Mar 12, 2020
@m2-community-project m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress Mar 12, 2020
tjwiebell
tjwiebell suggested changes Mar 12, 2020
View reviewed changes
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

Some minor tweaks and this should be good to go.

packages/venia-ui/lib/components/Header/cartTrigger.js Outdated
const CartTrigger = props => {
const { handleClick, itemCount } = useCartTrigger({
createCartMutation: CREATE_CART_MUTATION,
getCartDetailsQuery: GET_CART_DETAILS_QUERY
});

const iconColor = props.iconColor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to destructure here. I can see how you probably just copied the code style from the line below, but that is the one exception to the rule. We like to reserve classes for use in the component, so we do not destructure it off props.

Suggested change
const iconColor = props.iconColor;
const { iconColor } = props;

packages/venia-ui/lib/components/Header/cartTrigger.js Outdated
Comment on lines 22 to 38
const cartIcon =
itemCount > 0 ? (
<Icon
src={ShoppingCartIcon}
attrs={{
fill: iconColor,
stroke: iconColor
}}
/>
) : (
<Icon
src={ShoppingCartIcon}
attrs={{
stroke: iconColor
}}
/>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're making changes, lets clean this up:

const cartIconAttributes = {
    fill: itemCount ? iconColor : 'none',
    stroke: iconColor
};

// down in the return
<Icon src={ShoppingCartIcon} attrs={cartIconAttributes} />

@tjwiebell tjwiebell added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Mar 12, 2020
@m2-community-project m2-community-project bot moved this from Review in Progress to Changes Requested in Pull Request Progress Mar 12, 2020
@tjwiebell
Copy link
Contributor

Hey @larsroettig, I know we're in some odd times, but wanted to check in on this PR. Happy to help push this across the finish line if need be.

@larsroettig
Copy link
Member Author

larsroettig commented Mar 16, 2020 via email

@tjwiebell
Copy link
Contributor

@larsroettig - We'll finish this up internally, just keep the feedback in mind for future contributions. Thank you for this PR and helping answer questions in the Community Slack 😀

@m2-community-project m2-community-project bot moved this from Changes Requested to Reviewer Approved in Pull Request Progress Mar 17, 2020
@tjwiebell tjwiebell merged commit 8bcf191 into magento:develop Mar 19, 2020
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Mar 19, 2020
@tjwiebell tjwiebell moved this from In Review to Done in Community Backlog Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjwiebell tjwiebell tjwiebell requested changes

@sirugh sirugh sirugh approved these changes

@supernova-at supernova-at Awaiting requested review from supernova-at

Assignees

@tjwiebell tjwiebell

@sirugh sirugh

Labels
Partner: TechDivision partners-contribution pkg:venia-ui version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
Pull Request Progress
Done
Community Backlog
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@larsroettig @PWAStudioBot @tjwiebell @sirugh @magento-engcom-team @dpatil-magento