-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix Nested Anchor Tags #1397
Fix Nested Anchor Tags #1397
Conversation
… longer needs to be a link
✅ Deploy Preview for fractal-framework-dev 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.
Looks good for me, sir!
{t('details')} | ||
</Button> | ||
); | ||
return <Button variant="secondary">{t('details')}</Button>; |
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.
Effectively, this expandedView
thingy says that ProposalAction
is used within actual details page, where we need not only button but like exact components with hooks and logic. So this specific button supposed to be shown only on the list. That being said - this change looks good
</Button> | ||
</Link> | ||
); | ||
return <Button variant={showActionButton && canVote ? 'primary' : 'secondary'}>{label}</Button>; |
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 code is supposed to get to this return
statement only while rendering ProposalAction
in the proposals list - so this change looks good for me
Thank you for the review @mudrila ! @tomstuart123 @Da-Colon when you get a chance, please :) Given that @mudrila said this implementation looks good, I just went ahead and tested more thoroughly. The issues that I though would pop up, didn't! So yeah, this seems good to go from my end as well. The things to test:
|
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.
Nicely done 👍
Closes #1396
The
ProposalAction
buttons, which show up in proposal list cards, no longer needs to be a link.Testing:
On dev load up any DAO that has proposals. Notice that when the proposal list populates, there's an error in the console talking about how nested anchor tags are not allowed.
Then do the same on this branch, and notice how that error does not appear.
@mudrila I would love your eyes on this, specifically. There are three files where the
ProposalAction
component is instantiated:The first file,
ActivityGovernance.tsx
is the proposal card that shows up on the DAO Home and Proposals pages. That seems good.But I'm not sure about these other two files. It seems like
AzoriusDetails
andSnapshotProposalDetails
are both the actual details pages for the various types of proposals. TheProposalAction
button, in these pages, is being used to display the various "Vote" buttons, when those are relevant? So I probably don't want to just straight up disable their links, as I'm doing in this PR?I'm thinking that's the case, just want to get your take on it.