This repository has been archived by the owner on Mar 12, 2020. It is now read-only.
[AUCT-744] iOS request condition report #2050
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yuki24
reviewed
Jan 23, 2020
There was a problem hiding this comment.
So far so good 👍 I think @dblandin might want to review this PR as well since he just worked on a similar change in Reaction artsy/reaction#3099 and I see a lot of similar bits of code here.
Let me know if you need help for writing tests.
brainbicycle
changed the title
Remove redundant condition section Test artwork details condition section Remove unused context module in schema
…uest Allow modifying textAlign in Modal, center aligning lot condition request Remove unused import
Workaround for modal layout issues, tests for feature flag, see details Workaround modal top margin affecting layout Undo accidental padding change in modal Fix test add feature flag disabled test Type assertion as string for other list items
brainbicycle
changed the title
|
Alright this is finally ready for review:
Corresponding PR in reaction: Successfully requesting a condition report
Error on requesting a condition report
Notes:
|
ds300
reviewed
Feb 6, 2020
Use optional chaining for lab option Remove unused mutationMessage param, cleanup promise completion Add explicit type to state Consolidate loading state updates
dleve123
reviewed
Feb 6, 2020
| @@ -58,4 +59,89 @@ describe("Artwork Details", () => { | |||
| expect(component.text()).not.toContain("Frame") | |||
| expect(component.text()).not.toContain("Signature") | |||
| }) | |||
|
|
|||
| it("shows condition description if present and not biddable", () => { | |||
| const artworkDetailsInfo = { | |||
There was a problem hiding this comment.
minor: but is it possible to provide a type to this fixture? I think we would want to type with RequestConditionReport_artwork
There was a problem hiding this comment.
👍 I added a type for the fixtures, good idea! Here the type is ArtworkDetails_artwork and there isn't a test query (I didn't write this original test). However I am happy to refactor to use a test query if you want just let me know.
Use passed in relay environment Use passed in relay environment Updates tests with new relay prop Typifying artwork in artworkDetails tests Add relay prop to analytics tests
dleve123
reviewed
Feb 6, 2020
dleve123
reviewed
Feb 6, 2020
src/lib/Scenes/Artwork/Components/__tests__/RequestConditionReport-analytics-tests.tsx
Outdated
Show resolved
Hide resolved
Type safety in requestCondition report tests Add return type in requestConditionReport promise
ds300
approved these changes
Feb 7, 2020
|
🚀 PR was released in v1.21.40 🚀 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I don't think I can finish this PR this week, so I'm hoping @erikdstock or @yuki24 can take it over.