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

[CSGN-225] Choose the destination of artist consign taps based on your active tab #3402

Merged

Conversation

pepopowitz
Copy link
Contributor

This work depends on #3400. The only commits in this diff that are related to this PR are the last two.

Addresses https://artsyproduct.atlassian.net/browse/CSGN-225

This PR uses your currently selected tab to determine where to send you when you tap the artist consign button.

For users who are on the home or search tab (or anywhere that isn't sell), we'll continue to send them to the sell tab landing page:

artist-consign-1

If you're already in the sell tab, we push another landing page view onto your nav stack. This addresses the experience of tapping on something and not getting enough feedback about where you're going:

artist-consign-2

#trivial (but not really; it's just covered by another entry that's already in the changelog.)

@ArtsyOpenSource
Copy link
Contributor


Fails
🚫 Missing Test Files:
  • src/lib/Scenes/Consignments/__tests__/SellTabLanding-tests.tsx

If these files are supposed to not exist, please update your PR body to include "#skip_new_tests".

Generated by 🚫 dangerJS


RCT_EXPORT_METHOD(getSelectedTabName:(RCTPromiseResolveBlock)resolve rejecter:(RCTPromiseRejectBlock)reject)
{
return self.getSelectedTabName(resolve, reject);
Copy link
Member

Choose a reason for hiding this comment

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

Seems helpful 👍

@@ -17,6 +18,7 @@ export interface ArtistConsignButtonProps {
export const ArtistConsignButton: React.FC<ArtistConsignButtonProps> = props => {
const tracking = useTracking()
const buttonRef = useRef()
const selectedTabName = useSelectedTabName()
Copy link
Member

Choose a reason for hiding this comment

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

👌

Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

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

Crazy impressive PR @pepopowitz 💯 Everything is very clear and straightforward and was able to follow along nicely 👍 No substantive feedback from here.

default:
return @"Unknown";
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't think there is a much better solution with objective-C/C enums seems fine to me.

…selected tab

If you're on the sell tab, we'll push a new page onto your nav stack; everywhere else, we'll send you to the sell tab landing page.
@pepopowitz pepopowitz force-pushed the csgn-225-conditional-nav-for-artist-consign-button branch from 21de9a9 to 3d1758f Compare June 4, 2020 17:24
@pepopowitz pepopowitz changed the title [WIP] [CSGN-225] Choose the destination of artist consign taps based on your active tab [CSGN-225] Choose the destination of artist consign taps based on your active tab Jun 4, 2020
Copy link
Member

@jonallured jonallured left a comment

Choose a reason for hiding this comment

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

Nice one!!

@@ -286,6 +287,10 @@ - (void)updateRoutes
return [[ARNavigationController alloc] initWithRootViewController:submissionVC];
}];

[self.routes addRoute:@"/collections/my-collection/marketing-landing" handler:JLRouteParams {
Copy link
Member

Choose a reason for hiding this comment

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

Nice route! 🤩

src/lib/NativeModules/TopMenu.tsx Show resolved Hide resolved
@jonallured jonallured merged commit f0b6861 into master Jun 4, 2020
@jonallured jonallured deleted the csgn-225-conditional-nav-for-artist-consign-button branch June 4, 2020 18:33
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.

None yet

6 participants