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

New Partner Detail UI with Tab View #1464

Merged
merged 2 commits into from
Nov 3, 2023
Merged

Conversation

adam-soltech
Copy link
Contributor

Ticket

Design (I will be doing a separate review with product and design around this and have a few remaining questions for them)

Some thought that came up while doing this:

  • Should Partner cards be combined into one component
  • Should partner tables be combined into one component
  • Should some of the shared styles be abstracted at least
  • Is the Tabs folder structure OK? Would you prefer it is all flat like before? Something else?

In these cases I defaulted to not trying to combine these components. Even though they share some attributes and combining them would be a bit more DRY, I've seen these types of abstractions (esp. with tables being passed wildly different configs) make a confusing mess very quickly.

I do think it might be good to abstract the partner profile card, or maybe a generic flat style card since they share the same styles and more or less the same functionality (edit the card fields), but wanted to defer to the team/Carson before getting too clever.

@adam-soltech adam-soltech requested a review from CarsonF as a code owner August 10, 2023 03:10
@adam-soltech adam-soltech requested review from andrewmurraydavid and a team August 22, 2023 16:27
Copy link
Member

@andrewmurraydavid andrewmurraydavid left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. I found a small shadow annoyance and commented on it.

Copy link
Member

@andrewmurraydavid andrewmurraydavid left a comment

Choose a reason for hiding this comment

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

Please fix the shadow bug.

Copy link
Member

@andrewmurraydavid andrewmurraydavid left a comment

Choose a reason for hiding this comment

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

LGTM! Waiting on @CarsonF's review to merge.

Copy link
Member

@CarsonF CarsonF left a comment

Choose a reason for hiding this comment

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

Great start @adam-soltech! LMK if you have questions

src/components/DataButton/DataButton.tsx Outdated Show resolved Hide resolved
src/scenes/Partners/Detail/PartnerDetail.tsx Outdated Show resolved Hide resolved
src/scenes/Partners/Detail/PartnerDetail.tsx Outdated Show resolved Hide resolved
src/scenes/Partners/Detail/PartnerDetail.tsx Outdated Show resolved Hide resolved
src/scenes/Partners/Detail/PartnerDetail.tsx Outdated Show resolved Hide resolved
src/scenes/Partners/Detail/PartnerDetail.tsx Outdated Show resolved Hide resolved
@adam-soltech adam-soltech force-pushed the agl/partner-detail-new-ui branch from 076efa6 to 66bdad2 Compare October 20, 2023 23:04
@adam-soltech adam-soltech force-pushed the agl/partner-detail-new-ui branch 3 times, most recently from 82bc927 to 25583a8 Compare November 2, 2023 20:12
@CarsonF CarsonF force-pushed the agl/partner-detail-new-ui branch from 25583a8 to f681703 Compare November 3, 2023 23:39
@CarsonF CarsonF force-pushed the agl/partner-detail-new-ui branch from f681703 to ad5b9d4 Compare November 3, 2023 23:43
@CarsonF CarsonF force-pushed the agl/partner-detail-new-ui branch from ad5b9d4 to 77e8ef1 Compare November 3, 2023 23:44
@CarsonF CarsonF changed the title agl/partner detail new UI New Partner Detail UI with Tab View Nov 3, 2023
Copy link
Member

@CarsonF CarsonF left a comment

Choose a reason for hiding this comment

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

Thanks @adam-soltech!

@CarsonF CarsonF enabled auto-merge November 3, 2023 23:46
@CarsonF CarsonF merged commit f1eba4d into develop Nov 3, 2023
4 checks passed
@CarsonF CarsonF deleted the agl/partner-detail-new-ui branch November 3, 2023 23:54
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.

3 participants