Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Home Screen - Changed activity panel button styles #5496

Merged
merged 2 commits into from
Nov 3, 2020

Conversation

octaedro
Copy link
Contributor

This PR is a follow-up of the PR 5455, so a rebase will be necessary.

This PR changes the styles of the panel button "Manage all orders".

Screenshots

screenshot-www figma com-2020 10 28-16_59_03

Detailed test instructions:

  • Go to the Home screen.
  • Verify the button Manage all orders inside the Orders panel is visible.
  • Press Manage all orders and verify it's working as expected.

Changelog Note:

Dev: Home Screen - Changed activity panel button styles

@octaedro octaedro requested a review from a team October 28, 2020 20:18
@octaedro octaedro self-assigned this Oct 28, 2020
@samueljseay
Copy link
Contributor

@octaedro Just a thought that if you change this branch to be merging into #5455 instead of main then its easy to review the individual changes without any rebasing.

Then once both are approved just merge them starting with this one and then finishing with the other one.

@octaedro
Copy link
Contributor Author

octaedro commented Oct 29, 2020

Hi @samueljseay,

@octaedro Just a thought that if you change this branch to be merging into #5455 instead of main then its easy to review the individual changes without any rebasing.

Good question. A few months ago, I tried the approach you mentioned but the reviewers felt that it would have been easier to review if we created the branches from the depending branch and pointing to main. I think that each approach has its own pros and cons.
I'm not sure if it's helpful but with an URL like this you can see in the browser the difference between two chosen PRs.

@octaedro
Copy link
Contributor Author

Hi @samueljseay,

Chatting with @jeffstieler he said that chaining PRs work better for him too so I'll change the base of this and the other PRs

@octaedro octaedro changed the base branch from main to add/5238 October 29, 2020 16:57
@jeffstieler
Copy link
Contributor

@octaedro this doesn't seem to match the design:

Am I referencing the wrong design?

@jeffstieler jeffstieler added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed [Status] Needs Review labels Oct 29, 2020
@@ -224,6 +224,7 @@ class OrdersPanel extends Component {
{ cards }
<ActivityOutboundLink
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how much value the ActivityOutboundLink component really has... this is the only usage of it. Why don't we just get rid of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the fact that you're adding more code to this unused component to remove the icon rendering I agree that ditching it has some value here. It has a bit of styling associated with it, but that styling could just move to a class name on a <Link> that you render here instead? It would be great to keep removing unused code to trim the code base down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I changed it in the commit 57ae4f5

@octaedro
Copy link
Contributor Author

octaedro commented Oct 29, 2020

Hi @jeffstieler,

I know it's a little confusing, I think that the latest designs are here pbIJXs-ma-p2#comment-1017.

@jeffstieler
Copy link
Contributor

Sheesh, I was in Figma and on a different iteration. 🤦

Copy link
Contributor

@samueljseay samueljseay left a comment

Choose a reason for hiding this comment

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

Keen to see what you think about removing that component.

@@ -224,6 +224,7 @@ class OrdersPanel extends Component {
{ cards }
<ActivityOutboundLink
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the fact that you're adding more code to this unused component to remove the icon rendering I agree that ditching it has some value here. It has a bit of styling associated with it, but that styling could just move to a class name on a <Link> that you render here instead? It would be great to keep removing unused code to trim the code base down.

This commit removes the ActivityOutboundLink component
@octaedro
Copy link
Contributor Author

octaedro commented Nov 2, 2020

Thank you @jeffstieler and @samueljseay for your reviews. I addressed the change you mentioned.
Could you give this PR another look?

@octaedro octaedro added [Status] Needs Review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Nov 2, 2020
Copy link
Contributor

@samueljseay samueljseay left a comment

Choose a reason for hiding this comment

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

Great stuff @octaedro tests well and the code looks great. Thanks for removing that old component! 🧹 🚀

@octaedro octaedro merged commit 8c68810 into add/5238 Nov 3, 2020
@octaedro octaedro deleted the add/5238_panel_button branch November 3, 2020 15:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants