Feature/83/create datasummary with markdown#91
Conversation
|
@ekraffmiller can you please resolve the merge conflicts? Also, which branch/PR did you branch from? This will help us create a "compare" link that makes code review easier (just the smaller diff). Thanks. |
MellyGray
changed the base branch from
develop
to
feature/78-create-the-boilerplate-of-the-dataset-page
MellyGray
left a comment
MellyGray
left a comment
There was a problem hiding this comment.
I love seeing we are making progresses with the Dataset page!
I left some comments but overall this looks good to me
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
Overall this is looking good. I hope this is a stepping stone to someday supporting Markdown!
I left a few comments.
| title: 'Description', | ||
| description: 'this is the description field', | ||
| value: | ||
| 'This is the description field. This text is *italic* and this is **bold**. Here is an image  ' |
There was a problem hiding this comment.
Can we please add a link here as well? Links are probably the most common use case. Something like Here is [a link](https://dataverse.org).
| } | ||
| const { container } = render(<SanitizedHTML html={html} options={options} />) | ||
|
|
||
| expect(container.innerHTML).toContain('href="https://example.com"') |
There was a problem hiding this comment.
The actual innerHTML is this:
<div><a target="_blank" href="https://example.com">Example</a> </div>
Perhaps the test could also assert that "target" is kept?
There was a problem hiding this comment.
Yes, I can update the test, but I think it's a low priority since I am removing this element from the branch. We are going to use the Markdown component instead.
…ataset.ts, update MarkdownComponent.tsx for React 18
|
@MellyGray & @pdurbin I made all the changes from the comments, thanks. I will look at the merge conflicts tomorrow. Since I'm branching off of other feature branches, I may have questions about that. |
|
sprint kickoff
|
…into feature/83/create-datasummary-with-markdown
|
@ekraffmiller I didn't see the DataSummary component in story book. Probably I'm overlooking something obvious. I did see a "summary block" label in the sample dataset page. |
…th-markdown Feature/83/create datasummary with markdown
6 participants
What this PR does / why we need it:
Creates a DataSummary component for the Dataset Page
Which issue(s) this PR closes:
Special notes for your reviewer:
I created a new branch off of my feature branch, to switch from SanitizedHtml to markdown (in order to save the SanitizedHtml code). I hope that doesn't make reviewing the PR harder - let me know if there is a better manage the branching.
Suggestions on how to test this:
npm run storybook - view DataSummary component
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
no
Is there a release notes update needed for this change?:
no
Additional documentation: