Skip to content
This repository has been archived by the owner on Dec 21, 2023. It is now read-only.

Use the new Modal Screen HOC #1053

Merged
merged 9 commits into from Apr 27, 2022
Merged

Use the new Modal Screen HOC #1053

merged 9 commits into from Apr 27, 2022

Conversation

axeldelafosse
Copy link
Contributor

@axeldelafosse axeldelafosse commented Apr 27, 2022 •
edited

Why

We have a new Modal Screen (thanks to @gorhom) 🎉 This PR implements the new HOC on all modals. The modal screens are now very clean and all share the same logic

How

  • Replaced the previous implementation with the new one
  • Cleaned modals (Modal, ModalSheet and ModalScreen) implementation
  • Removed background color on web
  • TODO: review modal content design (Login, Followers / Following, Create)
  • TODO: implement Comments modal "No comments yet…" state

Test Plan

Run the app and check all the modals, on all platforms and all screen sizes

@vercel
Copy link

vercel bot commented Apr 27, 2022 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
showtime ✅ Ready (Inspect) Visit Preview Apr 27, 2022 at 1:19PM (UTC)

@github-actions
Copy link

github-actions bot commented Apr 27, 2022 •
edited

PR Preview - Storybook

This pull request preview deployment is now available.

✅ Preview: exp+io.showtime.storybook://expo-development-client/?url=https://u.expo.dev/e77d5b68-bb27-45da-aa5c-96c1fdbf6706&channel-name=pr-1053

Comment ID: 1110887695

@github-actions
Copy link

github-actions bot commented Apr 27, 2022 •
edited

PR Preview - Storybook on Chromatic

✅ Preview: The Storybook preview URL for this Pull Request

✅ Build: The Chromatic build URL for this Pull Request

@github-actions
Copy link

github-actions bot commented Apr 27, 2022 •
edited

PR Preview - App

This pull request preview deployment is now available.

✅ Preview: exp+io.showtime.development://expo-development-client/?url=https://u.expo.dev/45cbf5d5-24fe-4aa6-9580-acf540651abd&channel-name=pr-1053

Comment ID: 1110934196

@axeldelafosse axeldelafosse marked this pull request as ready for review April 27, 2022 12:38
intergalacticspacehighway
intergalacticspacehighway approved these changes Apr 27, 2022
View reviewed changes
Copy link
Collaborator

@intergalacticspacehighway intergalacticspacehighway left a comment

Choose a reason for hiding this comment

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

LGTM. Wow! looks clean. 💯

@alantoa
Copy link
Collaborator

alantoa commented Apr 27, 2022

LGTM! 👍

enesozturk
enesozturk reviewed Apr 27, 2022
View reviewed changes
packages/app/components/transfer.tsx
addSuffix: true,
}
)}`}
{nft?.token_created &&
Copy link
Contributor

Choose a reason for hiding this comment

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

As I know using && operator in renders might cause unexpected results. Not sure if it can be 0 (zero) but even if not, it would be good practice to use Ternary Operator for the values comes from the endpoint which we are not sure it will be 0 or undefined.

Ref: https://kentcdodds.com/blog/use-ternaries-rather-than-and-and-in-jsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, thanks! Is there an ESLint plugin for this? I keep forgetting about it

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there is but it's not specific to JSX, we should still be able to use the && operator. I'll check it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Included this change in my latest PR

enesozturk
enesozturk reviewed Apr 27, 2022
View reviewed changes
packages/design-system/modal-sheet/index.tsx
@@ -31,7 +31,7 @@ export function ModalSheet({ visible = true, bodyContentTW, ...props }: Props) {
<Modal
key={`modalsheet-${props.title}-lg`}
title={props.title}
close={() => {
onClose={() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👌🏽

@axeldelafosse axeldelafosse merged commit 6f672ee into staging Apr 27, 2022
@enesozturk
Copy link
Contributor

Screen components are looking very light weight now with the refactored withModalScreen HOC, Cool cool 👌🏽

@axeldelafosse axeldelafosse deleted the modal-screen branch May 3, 2022 13:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@enesozturk enesozturk enesozturk left review comments

@intergalacticspacehighway intergalacticspacehighway intergalacticspacehighway approved these changes

@alantoa alantoa alantoa approved these changes

@gorhom gorhom Awaiting requested review from gorhom

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@axeldelafosse @alantoa @enesozturk @intergalacticspacehighway