-
Notifications
You must be signed in to change notification settings - Fork 85
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
feat: Modal to publish worlds to ENS names #2903
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Pull Request Test Coverage Report for Build 6332064957
💛 - Coveralls |
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.
The PR looks good! I've left some comments that need some reviewing
}, [ensList, onRecord, analytics]) | ||
|
||
useEffect(() => { | ||
if (view === DeployToWorldView.FORM && loading && error) { |
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.
Should it show the error view while loading?
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.
This logic came from the original component.
What I understand from this is that when the user clicks on publish, loading is set to true.
Then when an error appears in the state, the useEffect picks it and changes the view to ERROR.
On the other case, if no error is provided but the current deployment is found on the store, the SUCCESS view is set.
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.
Answering your question: It does not show the error while loading
className={styles.modalBodyStateActionButton} | ||
content={t('deployment_modal.deploy_world.success.share_in_twitter')} | ||
icon="twitter" | ||
href={getShareInTwitterUrl()} |
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 could track the twitter sharing action if we would like to, what do you think about it?
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.
It might be a good idea to track it.
#2921 Created an issue in the epic to do so
}: Props) { | ||
const analytics = getAnalytics() | ||
|
||
const [view, setView] = useState<string>('') |
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.
Would you mind adding tests for this component?
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.
It's kind of a big component. It's a pity that original one didn't have tests.
Testing the whole component will require a lot of time and might break the schedule 😢
Co-authored-by: Lautaro Petaccio <[email protected]> Signed-off-by: Fernando Zavalia <[email protected]>
9f594ea
to
becd3a1
Compare
src/modules/ens/sagas.spec.ts
Outdated
it('should get the ens object by using the getENSBySubdomain selector', async () => { | ||
await expectSaga(ensSaga, builderClient, ensApi) | ||
.select(getENSBySubdomain, subdomain) | ||
.dispatch(fetchENSWorldStatusRequest(subdomain)) |
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.
The main objective of the sagas tests is to check the correctness of the actions that are put (which is the objective of any saga), would you mind checking what is put?
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.
🚀
Closes #2899
Closes #2906
Closes #2905