Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ME: form field wrapper, abstract record field #854

Merged
merged 6 commits into from
Apr 19, 2024
Merged

ME: form field wrapper, abstract record field #854

merged 6 commits into from
Apr 19, 2024

Conversation

LHBruneton-C2C
Copy link
Collaborator

@LHBruneton-C2C LHBruneton-C2C commented Apr 12, 2024 •
edited
Loading

Description

This PR introduces a new component to share the logic of adding a title and help tooltip to each record form field.

The form field for abstract is now specifically using the rich form field, based on markdown-editor and the new wrapper.

Architectural changes

Form fields have been move from ui-inputs to feature-editor.

Screenshots

image

Quality Assurance Checklist

  • Commit history is devoid of any merge commits and readable to facilitate reviews
  • If new logic ⚙️ is introduced: unit tests were added
  • If new user stories 🤏 are introduced: E2E tests were added
  • If new UI components 🕹️ are introduced: corresponding stories in Storybook were created
  • If breaking changes 🪚 are introduced: add the breaking change label
  • If bugs 🐞 are fixed: add the backport <release branch> label
  • The documentation website 📚 has received the love it deserves

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 12, 2024 •
edited
Loading

Affected libs: feature-editor, ui-elements, feature-catalog, feature-record, feature-router, feature-search, feature-map, feature-dataviz, ui-catalog, ui-search, ui-inputs, ui-layout,
Affected apps: datafeeder, metadata-editor, demo, datahub, webcomponents, map-viewer, search, metadata-converter,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 12, 2024 •
edited
Loading

📷 Screenshots are here!

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/form-field-wrapper-markdown-editor-dependencies branch 2 times, most recently from b5bef8e to 52d3902 Compare April 18, 2024 08:46
@LHBruneton-C2C LHBruneton-C2C mentioned this pull request Apr 18, 2024
Merged
8 tasks
@LHBruneton-C2C LHBruneton-C2C changed the title Me/form field wrapper markdown editor dependencies ME:form field wrapper, abstract record field Apr 18, 2024
@LHBruneton-C2C LHBruneton-C2C changed the title ME:form field wrapper, abstract record field ME: form field wrapper, abstract record field Apr 18, 2024
@LHBruneton-C2C LHBruneton-C2C marked this pull request as ready for review April 18, 2024 14:35
jahow
jahow approved these changes Apr 18, 2024
View reviewed changes
Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice refactoring, thanks! it makes a lot of sense to me.

When opening the editor I noticed that the rich text field wasn't adjusting its height correctly in preview mode, see:

image

Also the storybook stories for the form-field component are broken now.

I think we can merge this PR and revisit some small things about the display (size, icons) later to not make this PR too complex. I'm leaving it to you to decide what to do in this PR and what to do later 🙂

libs/feature/editor/src/lib/components/record-form/form-field/form-field.component.stories.ts Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These stories do not work anymore; maybe they're not relevant now?

@LHBruneton-C2C
Copy link
Collaborator Author

@jahow Thanks for the review!
I don't know if you've noticed, but the datafeeder is not building in production anymore, complaining about markdown-parser CSS being too large (I didn't change anything there, but it might be "affected").
Do you have any suggestion to fix or bypass this error?

@jahow
Copy link
Collaborator

jahow commented Apr 19, 2024

I don't know if you've noticed, but the datafeeder is not building in production anymore, complaining about markdown-parser CSS being too large (I didn't change anything there, but it might be "affected").
Do you have any suggestion to fix or bypass this error?

It might be an issue with bundle size budgets? This value should be bumped a bit:

geonetwork-ui/apps/datafeeder/project.json

Line 44 in 179fba8

"maximumError": "4kb"

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/form-field-wrapper-markdown-editor-dependencies branch from 52d3902 to b7b243f Compare April 19, 2024 08:53
@LHBruneton-C2C
Copy link
Collaborator Author

TODO: fix markdown-parser height.

@coveralls
Copy link

Coverage Status

coverage: 81.72% (-2.3%) from 83.981%
when pulling b7b243f on ME/form-field-wrapper-markdown-editor-dependencies
into 179fba8 on main.

@LHBruneton-C2C LHBruneton-C2C merged commit b46332e into main Apr 19, 2024
9 checks passed
@LHBruneton-C2C LHBruneton-C2C deleted the ME/form-field-wrapper-markdown-editor-dependencies branch April 19, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jahow jahow jahow approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LHBruneton-C2C @jahow @coveralls