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

Show prices on default view of artist auction results. #3572

Merged
merged 10 commits into from
May 21, 2020
Merged

Show prices on default view of artist auction results. #3572

merged 10 commits into from
May 21, 2020

Conversation

artsyjian
Copy link
Contributor

@artsyjian artsyjian commented May 20, 2020

https://artsyproduct.atlassian.net/browse/PURCHASE-1925

Currently on Artist Auction Results, user must be logged-in to see prices. This change is to relax that restriction but only for the default view which is the view on page 1 when filters are at default. We will show prices in the default view even when no user is logged-in.

Please note, due to filter context being incorrect about the page# of the page being viewed (it always says 1), I cannot track the page# the user is on. I resort to tracking whether pagination has occurred. If pagination has occurred, prices are hidden. So if user clicks page 1 while on page 1, the page hasn't changed, but pagination has occurred, prices will be hidden again.

--
Screenshots showing effects of the change:

Not logged-in. Page 1. Default filters. Price shown.
Screenshot from 2020-05-20 16-49-44

Not logged-in. Page 1. Medium Painting checked. Prices hidden.
Screenshot from 2020-05-20 16-50-33

Not logged-in. Page 2. Default filters. Prices hidden.
Screenshot from 2020-05-20 16-50-57

@artsy-peril artsy-peril bot added the Version: Patch Indicates that this PR should have a patch deploy, usually for bug fixes label May 20, 2020
…e we just need to know whether pagination occurred, not how many times it occurred.
@zephraph zephraph added Version: Minor Indicates that this PR should have a minor deploy, usually for new features and removed Version: Patch Indicates that this PR should have a patch deploy, usually for bug fixes labels May 21, 2020
Copy link
Contributor

@jpotts244 jpotts244 left a comment

Choose a reason for hiding this comment

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

Very cool - this LGTM!

Nice handling of some complicated and nuanced behavior!

artsyjian added 2 commits May 21, 2020 13:53
…naming it to 'filtersAtDefault' because that's really what it's about. Also moving it up a level in the component tree so it doesn't get evaluated 10 times at each render.
@zephraph
Copy link
Contributor

Amazing work @artsyjian. Thanks for the detailed comments and test updates as well. 👏

@zephraph zephraph merged commit d021d1d into artsy:master May 21, 2020
@artsyit
Copy link
Contributor

artsyit commented May 21, 2020

🚀 PR was released in v26.49.0 🚀

damassi added a commit to damassi/force that referenced this pull request May 22, 2020
damassi added a commit to artsy/force that referenced this pull request May 22, 2020
…to retire-reaction

* 'retire-reaction' of https://github.com/damassi/force: (23 commits)
  Disable codecov for jest for now
  [Migration] Comment out experiment viewed
  [Migration] Fix tracking event invocation
  Move artsy/reaction#3590
  [Migration] Update coffeescript /v2 paths to relative
  Move artsy/reaction#3587
  Move artsy/reaction#3589
  Move artsy/reaction#3576
  Move artsy/reaction#3572
  Rename .babelrc to babel.config.js
  [Migration] Finish migration
  [Migration] Migrate tests
  [Migration] Rename v2/*.test to .jest
  [Migration] Delete publishing folder
  [Migration] Update import paths
  [Migration] Enable incremental type-checking
  [Migration] Temporarily disable @types for relay
  [Migration] Storybooks
  [Migration] Add relay
  Fix type-check errors in force
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Version: Minor Indicates that this PR should have a minor deploy, usually for new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants