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/record field license #860

Merged
merged 5 commits into from
Apr 30, 2024
Merged

Me/record field license #860

merged 5 commits into from
Apr 30, 2024

Conversation

LHBruneton-C2C
Copy link
Collaborator

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

Description

This PR introduces support of the licenses field in the record form.

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

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/record-field-license branch from 2ea06c4 to 3fe0e15 Compare April 19, 2024 14:28
@github-actions GitHub Actions
Copy link
Contributor

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

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

  • 🚀 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 19, 2024 •
edited
Loading

📷 Screenshots are here!

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/record-field-license branch from 3fe0e15 to 3e34cce Compare April 25, 2024 11:02
@LHBruneton-C2C LHBruneton-C2C marked this pull request as ready for review April 25, 2024 11:05
@LHBruneton-C2C LHBruneton-C2C requested a review from jahow April 25, 2024 11:05
@coveralls
Copy link

Coverage Status

coverage: 81.801% (-2.3%) from 84.119%
when pulling 3e34cce on ME/record-field-license
into 139106e on main.

LHBruneton-C2C
LHBruneton-C2C commented Apr 26, 2024
View reviewed changes
libs/feature/editor/src/lib/fields.config.ts Outdated
{
model: 'licenses',
formFieldConfig: {
labelKey: 'editor.record.form.license',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO: marker + i18n

LHBruneton-C2C
LHBruneton-C2C commented Apr 26, 2024
View reviewed changes
...c/lib/components/record-form/form-field/form-field-license/form-field-license.component.html Outdated
@@ -0,0 +1,8 @@
<gn-ui-dropdown-selector
title="license"
Copy link
Collaborator Author

@LHBruneton-C2C LHBruneton-C2C Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

TODO: i18n
Edit: title not shown

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's in the tooltip I think, so no big deal

jahow
jahow approved these changes Apr 30, 2024
View reviewed changes
Copy link
Collaborator

@jahow jahow left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It looks really good! I think the model for the field configuration might not be fitting anymore, since formFieldConfig expects a "type" but the field shown is now more dependant on the model property instead.

This can be sorted out later on though. Thanks a lot!

...c/lib/components/record-form/form-field/form-field-license/form-field-license.component.html Outdated
@@ -0,0 +1,8 @@
<gn-ui-dropdown-selector
title="license"
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's in the tooltip I think, so no big deal

@LHBruneton-C2C
Copy link
Collaborator Author

It looks really good! I think the model for the field configuration might not be fitting anymore, since formFieldConfig expects a "type" but the field shown is now more dependant on the model property instead.

This can be sorted out later on though. Thanks a lot!

Yes, there will be some clean up to do, but it will be easier to deal with when no other field relies on it.

@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/record-field-license branch from 3e34cce to 8e14eeb Compare April 30, 2024 14:56
@LHBruneton-C2C LHBruneton-C2C force-pushed the ME/record-field-license branch from 8e14eeb to 5c3d277 Compare April 30, 2024 15:11
@LHBruneton-C2C LHBruneton-C2C merged commit e07c560 into main Apr 30, 2024
9 checks passed
@LHBruneton-C2C LHBruneton-C2C deleted the ME/record-field-license branch April 30, 2024 15:15
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 @coveralls @jahow