-
Notifications
You must be signed in to change notification settings - Fork 6
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
Remove collaboration tab #2365
Remove collaboration tab #2365
Conversation
b000ee5
to
d46acb4
Compare
You can access the deployment of this PR at https://renku-ci-ui-2365.dev.renku.ch |
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 works well!
I added a code suggestion to use standard components on the dropdown instead of linking the onClick
action to window.open
const onClick = () => window.open(url, "_blank"); | ||
return ( | ||
<DropdownItem onClick={onClick} size={size}> | ||
{text} | ||
</DropdownItem> | ||
); |
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.
window.open
doesn't allow the typical interactions offered by an anchor element.
In other similar scenarios, we use the ExternalLink
component with the nav-link
class. I suggest adopting the same solution here
const onClick = () => window.open(url, "_blank"); | |
return ( | |
<DropdownItem onClick={onClick} size={size}> | |
{text} | |
</DropdownItem> | |
); | |
return ( | |
<DropdownItem onClick={onClick} size={size}> | |
<ExternalLink className="nav-link" role="text" title={text} url={url} /> | |
</DropdownItem> | |
); |
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.
Good suggestion, I have incorporated 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.
🚀
Tearing down the temporary RenkuLab deplyoment for this PR. |
Screenshots
Testing
Fix #2256
/deploy #cypress #persist renku=renku-ui-3.2.0-tests