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.

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

351 add relationship info #505

Merged
merged 22 commits into from
Mar 13, 2024
Merged

351 add relationship info #505

merged 22 commits into from
Mar 13, 2024

Conversation

ofu997
Copy link
Contributor

@ofu997 ofu997 commented Nov 8, 2023

This PR:

Resolves #351

Screenshots (if applicable):

image

Additional Context (optional):

Add relation and relationship status to contact. We are currently (Nov 14) not sure whether we will set this in the add contact modal or from the table in the contact list.

Currently this creates two new constants files, and changes the useContactsList hook.

Future Steps/PRs Needed to Finish This Work (optional):

Add any other steps/PRs that may be needed to continue this work if this PR is just a step in the right direction.

Issues needing discussion/feedback (optional):

1. I'm unable to do the pod field which is the second field in the screenshot. add this field: pod: <podUrl>, // optional: the person's preferred pod url. If not present, we fetch the first pod url on the profile document. If that's not present, we derive it from the webid. When I tried to do it in previous commits there would be Promise or auth errors. This needs to be done later as a separate issue.

@leekahung leekahung added enhancement Enhancement of existing features discovery Open discussion and exploration of topic on hand labels Nov 8, 2023
@leekahung
Copy link
Contributor

Hey @ofu997! Thanks for your contribution to the project.

I have a general sense of what the changes are, but would like to have the PR description updated to detail what you've done/changed for this PR. It'll help let other users get a feel for what this PR is for.

Also, keep note of unused imports and other errors provided by the linter and update the codebase accordingly (see lint check on GitHub: https://github.com/codeforpdx/PASS/actions/runs/6794649087/job/18471386525?pr=505).

Copy link
Contributor

@leekahung leekahung left a comment

Choose a reason for hiding this comment

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

Hey @ofu997! Just gone through some of the code. It looks like this aims to add new fields to the contacts object that we'll be storing with the contactsList, which I think is good. However, I'm not too sure about adding them as adjustable fields in src/components/Modals/AddContactModal.jsx just yet. Think we'll need more discussion about how to implement these new properties.

There are also other thing that need addressing like the unit tests. See feedback below.

src/components/Modals/AddContactModal.jsx Outdated Show resolved Hide resolved
src/components/Modals/AddContactModal.jsx Outdated Show resolved Hide resolved
src/components/Modals/AddContactModal.jsx Outdated Show resolved Hide resolved
src/components/Modals/AddContactModal.jsx Outdated Show resolved Hide resolved
src/components/Modals/AddContactModal.jsx Outdated Show resolved Hide resolved
src/constants/relationship_status.js Outdated Show resolved Hide resolved
src/constants/relationships.js Outdated Show resolved Hide resolved
Copy link
Contributor

@leekahung leekahung left a comment

Choose a reason for hiding this comment

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

Made a few more change requests to this PR, think it should be good after getting those addressed.

src/constants/relationship_status.js Outdated Show resolved Hide resolved
src/components/Modals/AddContactModal.jsx Outdated Show resolved Hide resolved
src/constants/relationships.js Outdated Show resolved Hide resolved
@leekahung
Copy link
Contributor

Also, there seems to be linting errors. Do you run the lint or prettier in your branch? It doesn't seem like prettier is being picked up in your PR.

@ofu997
Copy link
Contributor Author

ofu997 commented Nov 15, 2023 via email

@leekahung
Copy link
Contributor

I ran npm run prettier:test without error.

We don't have a script for prettier:test, see screenshot.

Screen Shot 2023-11-15 at 13 06 15

npm run prettier:run is the script for running Prettier to update formatting. npm run prettier:check is the script for running Prettier to check formatting.

@ofu997
Copy link
Contributor Author

ofu997 commented Nov 17, 2023

Weird
image

@leekahung
Copy link
Contributor

Weird image

You need to run npm run prettier:run. For npm run prettier:check it's used by the CI to double check if formatting was missing.

Note the error found in the check:

Screen Shot 2023-11-16 at 21 17 50 (2)

@leekahung leekahung self-requested a review November 19, 2023 23:43
@leekahung
Copy link
Contributor

Hey @ofu997 ! Wanted to check on the progress of this PR.

Believe we could start getting the discussions for where to place these new fields with the UX team once we got this merged and start an issue opened for implementation.

Copy link
Contributor

@leekahung leekahung left a comment

Choose a reason for hiding this comment

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

Think it's good for merging. Think we could start an open discussion about how to utilize these new properties now that those statuses are now available.

Copy link
Contributor

@timbot1789 timbot1789 left a comment

Choose a reason for hiding this comment

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

The structure of the "role" property seems incorrect. We're storing a string to it, but the definition on schema.org describes an object (technically a "thing") with a few different attributes.

The documentation actually links to a blog post about it: http://blog.schema.org/2014/06/introducing-role.html

@ofu997
Copy link
Contributor Author

ofu997 commented Dec 7, 2023

I'll make it https://schema.org/roleName

@timbot1789
Copy link
Contributor

I'll make it https://schema.org/roleName

That looks like a good solution to me

@leekahung
Copy link
Contributor

Hey @ofu997! Was wondering if this is still being worked on. If you are, let us know. Thanks!

@ofu997
Copy link
Contributor Author

ofu997 commented Feb 20, 2024

Hey @ofu997! Was wondering if this is still being worked on. If you are, let us know. Thanks!

Hi Kahung,
It looks like I said "I'll make it https://schema.org/roleName" but didn't make a commit. I'm at work RN, so If it seems pretty much ready you can finish this up.

@timbot1789 timbot1789 merged commit b6a1c0b into Development Mar 13, 2024
3 checks passed
@leekahung leekahung deleted the 351-add-relationship-info branch April 20, 2024 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discovery Open discussion and exploration of topic on hand enhancement Enhancement of existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add relationship information to contacts
3 participants