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

spike: saving artworks to lists on other grids #11978

Merged
merged 19 commits into from
Mar 1, 2023

Conversation

dimatretyak
Copy link
Contributor

@dimatretyak dimatretyak commented Feb 24, 2023

The type of this PR is: Spike

Review app: artwork-saving

Demo for saves page

demo-1.mp4

Demo for artist artworks page

demo-2.mp4

@dimatretyak dimatretyak self-assigned this Feb 24, 2023
Comment on lines +69 to +75
{!!artworkEntityId && (
<SelectListsForArtworkModalQueryRender
artworkID={artworkEntityId}
onClose={clearArtworkId}
onSave={handleSaveListsForArtwork}
/>
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This approach to displaying the modal helps us solve this and some other problems

@dimatretyak dimatretyak changed the title spike: artwork saving spike: saving artworks to lists on other grids Feb 28, 2023
showHoverDetails={showHoverDetails}
disableRouterLinking={disableRouterLinking}
to={to}
enableSaveButtonForLists
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the future, this prop will not be required and these changes will be propagated to all components (FlatGridItem, ShelfArtwork, etc)

fragment SaveArtworkToListsButton_artwork on Artwork {
id
internalID
is_saved: isSaved
Copy link
Member

Choose a reason for hiding this comment

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

I think we can change that to:

Suggested change
is_saved: isSaved
isSaved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you don't mind, then I think it's better to do it in a separate PR and replace it in all places at once (e.g here, here, here, etc)

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.

Looks great 👍 My only question would be the toast inside the artwork, vs at the standard bottom right of the page, but that's probably been settled by design.

@dzucconi
Copy link
Member

@damassi the toast is top center in Force actually

@damassi
Copy link
Member

damassi commented Feb 28, 2023

Ah, I thought that was only in a few spots, but most positions its at the bottom right (like in our other apps). Hm

@anandaroop
Copy link
Member

anandaroop commented Feb 28, 2023

Just earlier today Ryan told me that design team has signed off on a global change to position the toasts in Force towards the bottom right of the viewport.

Does anyone foresee complications?

anandaroop
anandaroop previously approved these changes Feb 28, 2023
Copy link
Member

@anandaroop anandaroop left a comment

Choose a reason for hiding this comment

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

LGTM

src/Components/Artwork/ManageArtworkForSaves.tsx Outdated Show resolved Hide resolved
src/Components/Artwork/Details.tsx Show resolved Hide resolved
Co-authored-by: Anandaroop Roy <anandaroop.roy+github@gmail.com>
@dimatretyak
Copy link
Contributor Author

Just earlier today Ryan told me that design team has signed off on a global change to position the toasts in Force towards the bottom right of the viewport

it is useful to know this. thank you for sharing this info!

@dimatretyak dimatretyak merged commit 852303a into main Mar 1, 2023
@dimatretyak dimatretyak deleted the dimatretyak/sandbox/saves branch March 1, 2023 13:44
@artsy-peril artsy-peril bot mentioned this pull request Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants