-
Notifications
You must be signed in to change notification settings - Fork 35
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
[2024-11-07] Show related contacts in the POI form #3162
Conversation
Code Climate has analyzed commit cba01b2 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 82.4% (0.0% change). View more on Code Climate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I have some questions:
- Should archived contacts be shown in the related contacts box? IMO we should at least note if they are archived, or hide them completely
- Issue [2024-11-07] Remove E-mail, phone number and website from POI #3086 mentions a
primary contact
that we will deliver in the api. Do you think it makes sense to highlight the primary contact also in the related contacts box? - Should the box be positioned closely to the existing contact details box or should we leave that up to [2024-11-07] Remove E-mail, phone number and website from POI #3086?
integreat_cms/cms/templates/pois/poi_form_sidebar/related_contacts_box.html
Outdated
Show resolved
Hide resolved
The box is currently hidden for users except Service Team and CMS Team, as the contact feature itself is hidden. |
c447e32
to
6fca83d
Compare
I've added "(⚠ Archived)" to the archived contacts.
@osmers |
Sorry, I did not see the questions before :) I think it makes sense to highlight the primary contact - maybe naming it something like general contact information? And in my understanding the box showing related contacts should substitute the existing contact details box - if substituting is not possible then place it just below or above the existing box so that once we delete the existing box, it will basically be in the same position :) |
7054563
to
9d1eac0
Compare
@david-venhoff @osmers The box of related contacts is moved up to directly under the contact data box, which we can remove after or during #3086 The primary contact is shown as "General contact information". It appears instead of "title + name" display. |
9d1eac0
to
c241298
Compare
This PR is currently being adjusted for the changes lately introduced. |
231b9ab
to
0e2fd29
Compare
Currently the related contacts are shown like this: Archived contacts are marked as suggested before, "General contact information" for primary contacts (as discussed already), and <point of contact for (title)> (+ name if name is given) for all other contacts. I know this schema is different from what introduced in #3142 (Contact for [NAME_OF_LOCATION] with [ATTRIBUTE]: [ATTRIBUTE]). [NAME_OF_LOCATION] is omitted because it is on the POI form and therefore it's clear about which POI it is. E-mail, phone number or website will not be shown here, for title (point of contact for) and name (if given) are easier to understand what contact it is (in my opinion). Is this way Okay for service team? |
In theory (not showing the POI) yes, but what happens when no name is provided and only a title (which might be double)? Do we then add the phone, email or webseite? Not sure if this will happen often, it's probably an edge case, but who knows what the municipalities will come up with. |
@osmers |
0e2fd29
to
c4948de
Compare
@osmers |
Sorry @MizukiTemma |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! I just have three minor points, otherwise it looks good to me
integreat_cms/cms/templates/pois/poi_form_sidebar/related_contacts_box.html
Outdated
Show resolved
Hide resolved
edada63
to
0c1f481
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, very nice!
The only thing missing I think is that the new contact box should replace the existing contact details box
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, looks good!
It'll be done as a part of #3086 |
a379d2d
to
6786b6a
Compare
Co-authored-by: David Venhoff <[email protected]> Co-authored-by: jarlhengstmengel <[email protected]>
6786b6a
to
bca251a
Compare
Short description
This PR implements a box into the POI form which shows contacts referring the POI.
Proposed changes
Side effects
Resolved issues
Fixes: #3087
Pull Request Review Guidelines