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

Add editable label directive to UI module #790

Merged
merged 3 commits into from
Feb 9, 2024
Merged

Add editable label directive to UI module #790

merged 3 commits into from
Feb 9, 2024

Conversation

LHBruneton-C2C
Copy link
Collaborator

@LHBruneton-C2C LHBruneton-C2C commented Feb 2, 2024 •
edited
Loading

Description

This PR introduces a new directive, to edit in place fields like titles and names.

Architectural changes

Strangely enough, the RecordFormComponent was declared as standalone, but not importing anything more than CommonModule. Had to at least import UiInputsModule for it to work properly.

Added the new EditableLabelDirective to the UiInputsModule too.

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 UI components 🕹️ are introduced: corresponding stories in Storybook were created
  • The documentation website 📚 has received the love it deserves

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 2, 2024 •
edited
Loading

Affected libs: feature-editor, ui-inputs, feature-dataviz, feature-record, feature-router, feature-search, feature-map, ui-elements, feature-catalog, ui-catalog, ui-search,
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

@LHBruneton-C2C
Copy link
Collaborator Author

@jahow can you confirm this is the technical approach you had in mind ?
If the technical approach is OK, what would be the proper way to deliver this feature? With a story into StoryBook only?
This is only a draft, I will complete with further tests and some refactoring / renaming.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 8, 2024 •
edited
Loading

GitHub Pages links:

  • (Documentation)[https://geonetwork.github.io/geonetwork-ui/GSGNEDIT-53-ui-component-editable-label/docs/]
  • (Demo & web components)[https://geonetwork.github.io/geonetwork-ui/GSGNEDIT-53-ui-component-editable-label/demo/]
  • (UI components storybook)[https://geonetwork.github.io/geonetwork-ui/GSGNEDIT-53-ui-component-editable-label/storybook/demo/]

@jahow
Copy link
Collaborator

jahow commented Feb 8, 2024

@jahow can you confirm this is the technical approach you had in mind ? If the technical approach is OK, what would be the proper way to deliver this feature? With a story into StoryBook only? This is only a draft, I will complete with further tests and some refactoring / renaming.

Yes I also like the approach of a directive, it gives more freedom for styling the affected label. I would suggest trying to use the element inner text as initalValue input, this would also make it easier to apply this directive conditionnally.

Having a complete story on storybook is enough to validate and merge this. Thanks!

@LHBruneton-C2C LHBruneton-C2C force-pushed the GSGNEDIT-53-ui-component-editable-label branch from 1e10eae to 2884457 Compare February 9, 2024 12:47
@LHBruneton-C2C LHBruneton-C2C changed the title GSGNEDIT-53: wip Add editable label directive to UI module Feb 9, 2024
@LHBruneton-C2C LHBruneton-C2C marked this pull request as ready for review February 9, 2024 12:50
@coveralls
Copy link

coveralls commented Feb 9, 2024 •
edited
Loading

Coverage Status

coverage: 82.294% (-1.7%) from 83.953%
when pulling eaf2a1f on GSGNEDIT-53-ui-component-editable-label
into 80d7a27 on main.

jahow
jahow approved these changes Feb 9, 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.

Thanks, simple and efficient! I just noticed an issue with the original content of the lael being prepended and appended with spaces. Feel free to merge when this is sorted out :)

libs/ui/inputs/src/lib/editable-label/editable-label.directive.ts Outdated
this.renderer.setStyle(appendedInput, 'text-decoration', 'inherit')

const hostContent = this.el.nativeElement.innerHTML
this.renderer.setProperty(appendedInput, 'value', hostContent)
Copy link
Collaborator

Choose a reason for hiding this comment

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

hostContent needs to be trimmed, otherwise there will be blanks in the first and last positions

Copy link
Collaborator

Choose a reason for hiding this comment

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

(or innerText could be used instead, I'm not sure if that would change anything)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well spotted! As we decided this should be specific to text elements, I'll go with textContents, and trim().

@LHBruneton-C2C LHBruneton-C2C force-pushed the GSGNEDIT-53-ui-component-editable-label branch from 2884457 to eaf2a1f Compare February 9, 2024 14:37
@LHBruneton-C2C LHBruneton-C2C merged commit 0da0455 into main Feb 9, 2024
9 checks passed
@LHBruneton-C2C LHBruneton-C2C deleted the GSGNEDIT-53-ui-component-editable-label branch February 9, 2024 16:30
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