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

feature(DH): Add more contact information #748

Merged
merged 7 commits into from
Jan 16, 2024
Merged

feature(DH): Add more contact information #748

merged 7 commits into from
Jan 16, 2024

Conversation

Angi-Kinas
Copy link
Collaborator

@Angi-Kinas Angi-Kinas commented Jan 4, 2024 •
edited
Loading

This PR adds more contact information in the metadata contact section if it exists.

It will look like this:
image

image

image

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 4, 2024 •
edited
Loading

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

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

@coveralls
Copy link

coveralls commented Jan 4, 2024 •
edited
Loading

Coverage Status

coverage: 86.319% (+0.4%) from 85.968%
when pulling 9e13f82 on DH-show-more-contact-information
into c15a456 on main.

cmoinier
cmoinier requested changes Jan 5, 2024
View reviewed changes
Copy link
Collaborator

@cmoinier cmoinier 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.

Thanks for the work 🙂
I tested and it works well with all of my datasets.

Only UI thing to fix IMO is the spacing between the address and the email in case there is no website, i.e. without website (too narrow) :

image

and with website :

image

I also would like to wait for a second opinion about the risk of having addresses without commas (if there is no risk then I will approve the PR) 🙂

libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.spec.ts Outdated
test.each(addressCases)(
'adressString: $address',
({ address, expectedResult }) => {
console.log(component)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Forgot to delete this console.log 🙂

libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.ts Outdated
@@ -34,6 +34,37 @@ export class MetadataContactComponent {
)
}

parseAddress(inputAddress) {
const addressParts = inputAddress.split(',').map((part) => part.trim())
Copy link
Collaborator

@cmoinier cmoinier Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The only thing that worries me is if one address doesn't have any commas (as we spoke about). Is this a case that can happen? @fgravin @jahow

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes definitely, there's no rule for addresses as far as I know; but then it'll simply end up being a long line, so we just need to make sure that the text overflow works well :) (looks to be the case in my tests)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay in this case I will keep it simple as you were proposing @jahow and only split by ',' and if that's not there, it will be a long line of text which overflows, as you said. I implemented it to fit the design, but I guess there are too many different variations of the user input.

fgravin
fgravin reviewed Jan 6, 2024
View reviewed changes
Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

A similar work has been started in #575
Dunno if it helps.

libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.html Outdated
<div *ngIf="contacts[0].address" class="mb-2">
<div class="flex">
<mat-icon
class="material-symbols-outlined !w-[20px] !h-[20px] !text-[20px] opacity-75"
Copy link
Member

Choose a reason for hiding this comment

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

Please use built-in classes like w-5 instead, when possible.

@Angi-Kinas Angi-Kinas force-pushed the DH-show-more-contact-information branch from fff7713 to cb35be1 Compare January 9, 2024 13:17
jahow
jahow reviewed Jan 11, 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.

Added some more comments, thanks for the contribution!

libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.html Outdated Show resolved Hide resolved
libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.html Outdated Show resolved Hide resolved
libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.html Outdated Show resolved Hide resolved
libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.html Outdated Show resolved Hide resolved
libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.html Outdated
>
<div class="flex flex-col ml-2">
<p class="text-sm">
{{ contacts[0].firstName }} {{ contacts[0].lastName }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might render weirdly if only one of the two is defined

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think now it should be handled correctly.

libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.html Outdated Show resolved Hide resolved
libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.html Outdated Show resolved Hide resolved
libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.html Outdated Show resolved Hide resolved
libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.ts Outdated
@@ -34,6 +34,37 @@ export class MetadataContactComponent {
)
}

parseAddress(inputAddress) {
const addressParts = inputAddress.split(',').map((part) => part.trim())
Copy link
Collaborator

Choose a reason for hiding this comment

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

yes definitely, there's no rule for addresses as far as I know; but then it'll simply end up being a long line, so we just need to make sure that the text overflow works well :) (looks to be the case in my tests)

libs/ui/elements/src/lib/metadata-contact/metadata-contact.component.ts Outdated Show resolved Hide resolved
jahow
jahow approved these changes Jan 15, 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 for the improvements! As discussed, the layout can probably be slightly improved using grid and gap instead of mb classes. I'm approving this, please merge when you feel this is ready!

@Angi-Kinas Angi-Kinas force-pushed the DH-show-more-contact-information branch from 7a8cc67 to 2a5630d Compare January 15, 2024 14:50
@Angi-Kinas Angi-Kinas merged commit 2446b01 into main Jan 16, 2024
8 checks passed
@Angi-Kinas Angi-Kinas deleted the DH-show-more-contact-information branch January 16, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fgravin fgravin fgravin left review comments

@cmoinier cmoinier cmoinier requested changes

@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.
5 participants
@Angi-Kinas @coveralls @fgravin @jahow @cmoinier