add ui element markdown-parser and markdown support on metadata… #746

cdebarros · 2024-01-02T13:14:23Z

Hi, first of all happy new year to everyone seeing this. Here is my first contribution to the project.

This pull request makes the following changes :

Fixes isssue [IGN] [Datahub] Add markdown support on metadata-info component #733

I used marked to parse markdown content to html and then i defined some basic style for the following elements as suggested by @jahow :

h1 to h6
ul, ol and li
p
a
blockquote
pre - code
hr
table
img

I tried to use tailwind base colors to make it close to the default style.
I'm not sure how should i render links, blockquotes or table. I'll wait for your feedbacks to make some changes if needed.

fgravin

Thanks @cdebarros for this first PR.

I need to wrap up the approach a bit with @jahow but as a first review, I would just point out that a component seems not appropriate for such a process.

I would have more seen a Pipe and/or a directive.

Also, I wonder if the css could be encapsulated somehow instead of being spread via the encapsulation: ViewEncapsulation.None option.

jahow · 2024-01-11T10:25:12Z

I need to wrap up the approach a bit with @jahow but as a first review, I would just point out that a component seems not appropriate for such a process.

I would have more seen a Pipe and/or a directive.

What would be an argument for using a pipe or directive instead of a component? I'm still unsure which solution is the best personally. Using a component has the advantage that we can use an encapsulated style inside it.

jahow

Thanks for this contribution! I think this looks really good. I added a few comments, specifically to avoid disabling the view encapsulation; once addressed I think this can be merged. Please let me know if you need any more information!

@fgravin I've thought about it again and using a component seems right to me; please let us know if you have arguments against it

jahow · 2024-01-15T09:01:52Z

libs/ui/elements/src/lib/markdown-parser/markdown-parser.component.css

+.markdown-body h1 {
+  margin: 0.67em 0;
+  font-weight: 600;
+  padding-bottom: 0.3em;
+  font-size: 2em;
+  color: var(--color-primary);
+}


The title styles currently are inconsistent with other titles, see:

I would suggest using @apply text-title font-title for all hx classes above

libs/ui/elements/src/lib/markdown-parser/markdown-parser.component.ts

jahow · 2024-01-15T09:30:06Z

libs/ui/elements/src/lib/markdown-parser/markdown-parser.component.ts

+  selector: 'gn-ui-markdown-parser',
+  templateUrl: './markdown-parser.component.html',
+  styleUrls: ['./markdown-parser.component.css'],
+  encapsulation: ViewEncapsulation.None,


you can get rid of this and leave the default view encapsulation by adding :host /deep/ before all classes in the CSS file; this is a bit more verbose but IMO essential since disabling view encapsulation is risky in the long run

jahow · 2024-01-15T09:33:58Z

Note: Snyk workflow always fails for forks; please disregard that

cdebarros · 2024-01-15T10:13:35Z

Thanks for your reviews !
i will try to make these changes when i have time, probably before the end of this week.

cdebarros · 2024-01-16T10:45:44Z

I just updated my contribution. Added css encapsulation and storybook for the component.

On the default storybook content, i explain which markdown elements are supported for now.

jahow

Thanks for the adaptations, I think this looks better now. There are choices in the styling of the output that might not fit for every occasion and theme, but for now we can keep it like that and see how it behaves in the "wild".

I realized there was no unit test, I missed that on the first review. Could you please add one that ensures that the input is processed into the div inner HTML?

Also the branch needs to be rebased; there will likely be a conflict because we added gnUiLinkify on the abstract in the meantime; this directive can be removed from the abstract since marked does the same thing :)

Thank you for your continued efforts

jahow · 2024-01-16T12:06:49Z

libs/ui/elements/src/lib/markdown-parser/markdown-parser.component.spec.ts

+
+  it('should create', () => {
+    expect(component).toBeTruthy()
+  })


Oh sorry, I didn't realize that in the first review but we need at least one test here, showing that a markdown input will be correctly transformed to HTML; this doesn't have to be exhaustive as we're not testing the underlying library, but simply making sure that the input is processed.

Could you please add that?

Oh, i totally forgot about that.

I created 3 tests described like this :

should render markdown as HTML

should render HTML as HTML

should render text as HTML

Is it too much ? Maybe the first test is enough.

That does look like a perfect amount of tests 🙂

jahow · 2024-01-16T12:17:49Z

libs/ui/elements/src/lib/markdown-parser/markdown-parser.component.ts

+@Component({
+  selector: 'gn-ui-markdown-parser',
+  templateUrl: './markdown-parser.component.html',
+  styleUrls: ['./markdown-parser.component.css'],


I also forgot to point out that we should always use changeDetection: ChangeDetectionStrategy.OnPush, to make sure that the change detection does not run too often

…-info

cdebarros · 2024-01-17T16:38:41Z

I added tests and the changeDetectionStrategy to the component. Also i rebased the branch.
Let me know if it needs other changes.

coveralls · 2024-01-17T16:57:34Z

coverage: 83.965% (-1.7%) from 85.709%
when pulling 17da947 on IGNF:markdown-support
into f866474 on geonetwork:main.

jahow

Thanks for this great contribution! There is a failure on the build attempts in the CI, to fix it you can bump the CSS budgets for all apps like so:

diff --git a/apps/datafeeder/project.json b/apps/datafeeder/project.json
index 67998c55..b22bc758 100644
--- a/apps/datafeeder/project.json
+++ b/apps/datafeeder/project.json
@@ -40,8 +40,8 @@
             },
             {
               "type": "anyComponentStyle",
-              "maximumWarning": "2kb",
-              "maximumError": "4kb"
+              "maximumWarning": "10kb",
+              "maximumError": "20kb"
             }
           ],
           "fileReplacements": [

cdebarros · 2024-01-18T09:39:23Z

I updated it for the 3 projects that have failed, it should be fine now.

Is there a way to remove warnings from the css file ? (there is the same warning in the carousel component)

jahow · 2024-01-18T10:25:37Z

Don't worry about the CSS warnings, I'll fix them in another PR.

Thank you again for this awesome contribution, congratulations on it and we're looking forward to more of this!! 🎉

cdebarros changed the title ~~[add] the ui element markdown-parser and markdown support on metadata…~~ add ui element markdown-parser and markdown support on metadata… Jan 2, 2024

fgravin added enhancement Proposal for a new feature good first issue Good for newcomers labels Jan 9, 2024

jahow added this to the 2.2.0 milestone Jan 10, 2024

fgravin reviewed Jan 10, 2024

View reviewed changes

jahow reviewed Jan 15, 2024

View reviewed changes

jahow reviewed Jan 16, 2024

View reviewed changes

cdebarros added 3 commits January 17, 2024 17:31

[add] the ui element markdown-parser and markdown support on metadata…

2e4fa87

…-info

Css is now encapsulated and storybook added for the component

3365932

add testing and changeDirectionStrategy

0b93325

cdebarros force-pushed the markdown-support branch from a784302 to 0b93325 Compare January 17, 2024 16:32

jahow approved these changes Jan 17, 2024

View reviewed changes

update file size limit for components

17da947

jahow merged commit 4983265 into geonetwork:main Jan 18, 2024
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ui element markdown-parser and markdown support on metadata… #746

add ui element markdown-parser and markdown support on metadata… #746

cdebarros commented Jan 2, 2024 •

edited

Loading

fgravin left a comment

jahow commented Jan 11, 2024

jahow left a comment

jahow Jan 15, 2024

jahow Jan 15, 2024

jahow commented Jan 15, 2024

cdebarros commented Jan 15, 2024

cdebarros commented Jan 16, 2024

jahow left a comment

jahow Jan 16, 2024 •

edited

Loading

cdebarros Jan 16, 2024

jahow Jan 17, 2024

jahow Jan 16, 2024

cdebarros commented Jan 17, 2024

coveralls commented Jan 17, 2024 •

edited

Loading

jahow left a comment

cdebarros commented Jan 18, 2024 •

edited

Loading

jahow commented Jan 18, 2024

add ui element markdown-parser and markdown support on metadata… #746

add ui element markdown-parser and markdown support on metadata… #746

Conversation

cdebarros commented Jan 2, 2024 • edited Loading

fgravin left a comment

Choose a reason for hiding this comment

jahow commented Jan 11, 2024

jahow left a comment

Choose a reason for hiding this comment

jahow Jan 15, 2024

Choose a reason for hiding this comment

jahow Jan 15, 2024

Choose a reason for hiding this comment

jahow commented Jan 15, 2024

cdebarros commented Jan 15, 2024

cdebarros commented Jan 16, 2024

jahow left a comment

Choose a reason for hiding this comment

jahow Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

cdebarros Jan 16, 2024

Choose a reason for hiding this comment

jahow Jan 17, 2024

Choose a reason for hiding this comment

jahow Jan 16, 2024

Choose a reason for hiding this comment

cdebarros commented Jan 17, 2024

coveralls commented Jan 17, 2024 • edited Loading

jahow left a comment

Choose a reason for hiding this comment

cdebarros commented Jan 18, 2024 • edited Loading

jahow commented Jan 18, 2024

cdebarros commented Jan 2, 2024 •

edited

Loading

jahow Jan 16, 2024 •

edited

Loading

coveralls commented Jan 17, 2024 •

edited

Loading

cdebarros commented Jan 18, 2024 •

edited

Loading