-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add Welcome component #836
Conversation
✅ Deploy Preview for acre-dapp-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for acre-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left review. Overall looks good. Please use style props shorthands (eg. w
instead of width
) and style tokens (eg. 2.5
instead of "10px"
) if possible (indicated below).
Use react router loaders to display modals in correct order. We want to display welcome modal after access code modal in standalone app and before wallet connection in embedded apps.
To emphasize the fragment of text.
Remove border based on the Figma views - now none of the modals have a border.
Pull up the `WelcomeModal/index.ts` file one level higher - keep the component code in `src/components/WelcomeModal.tsx` file.
Display conntent based on the embedded app.
Use `mp4` files.
Set correct border radius for the video box. We should make rounded corners only on the right side to fit the modal dialog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great 🚀 I left some minor comments.
// Cast to fix eslint error: `unbound-method`. | ||
const { activeStep, goToNext } = useSteps({ | ||
index: 0, | ||
count: steps.length, | ||
}) as UseStepsReturn & { goToNext: () => void } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably update the Chakra package, see here. But let's leave it as it is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. But it's not a good time to update main packages just before the release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add it to our backlog.
Remove no longer needed border styles. We do not want to display border for the modal component.
We already have a file for router utils. Let's keep everything in one file.
There is no need to send the request to acre api to verify access code if the access code from local storage is `undefined` - it means the access code is invalid and we can skip the validation.
In some cases we render different content layouts etc depnding on whether it is an embedded or standalone application. Here we introduce the `DappMode` type to simplify code and organize the dapp modes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
The requests have been implemented.
Closes: #824
Closes: AENG-1
This PR adds a
Welcome
component based on the Figma views. Here we are also updating how we display modal in the correct order - we use react router loaders to display modals in the correct order. We want to display the welcome component after the access code modal in a standalone app and before the wallet connection in embedded apps.Screenshots