-
Notifications
You must be signed in to change notification settings - Fork 82
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
Enable hyperlinks in dataset description #1591
Conversation
@@ -0,0 +1,33 @@ | |||
export const createLinkMarkup = (text, color) => { |
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.
nit: Is the color ever going to change? If not, could we get it from the theme directly in the linkMarkup util?
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.
I did look into this. The color does need to change when you swap from light to dark theme in the settings. But eslint doesn't like it when the useTheme hook is called within linkMarkup as createLinkMarkup isn't a React component or custom hook. I looked into wrapping the function in a component, but then calling that started to get a little hacky just for the purpose of avoiding the param.
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.
Understood, seems like the solution is more complex than the problem itself. I think we can leave it as is.
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.
Hi @rbernotas, left some minor comments, but the functionality looks good. Thanks for the PR
integration tests had a failure in test_submit_share_extension_request_with_auto_approval. The error doesn't look related to this code change... is this a known issue? |
Yes, there was a small issue on the expiration date calculation for the last day of a month. The fix is in this PR. I re-run the failed job for this PR because today, 1st of October, we don't run into the issue |
Feature or Bugfix
Detail
Relates
Security
Please answer the questions below briefly where applicable, or write
N/A
. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)? Dataset description field is parsed for hyperlinks
eval
or similar functions are used? YesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.