Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Add extended field handling to facility details #1593

Merged
merged 1 commit into from
Jan 19, 2022
Merged

Conversation

TaiWilkin
Copy link
Contributor

@TaiWilkin TaiWilkin commented Jan 18, 2022

Overview

Makes the facility details subsections expandable to show additional
content (details from other matches, claims, etc.) when not in embed
mode.

Adds extended field subsections (number of workers, native language
name, and so on) when values are present and the map is not in embed
mode.

Adds nonstandard/contributor field subsections when values are present
in embed mode.

Connects #1540

Demo

Closed items
Screen Shot 2022-01-18 at 2 02 03 PM

Opened items
Screen Shot 2022-01-18 at 2 02 12 PM

Extended fields
Screen Shot 2022-01-18 at 2 02 59 PM

Nonstandard / contributor fields (in embed)
Screen Shot 2022-01-18 at 2 21 50 PM

Notes

Anticipated extended fields have been included in the list of fields to attempt to render if available, in addition to the existing extended fields. However, because these are not yet present in the API response, some tweaking might be necessary once they are being returned from the API to ensure all formatting / etc are working as expected.

Some changes have been made to the styling of the items and header based on the updated wireframes.

Testing Instructions

  • Run ./scripts/server
  • View a facility with multiple matches and confirm that other names, addresses, and GPS coordinates are available when expanding the subsections.
  • Submit and approve a claim for the facility, then submit a claim name, address, and number of workers.
  • The new claim details should be displayed in the expandable lists of facility details.
  • Submit and setup nonstandard fields for the facility.
  • Open the facility details in the embedded map preview. No items should be expandable, the contributors sections and expanded fields should be hidden, and the nonstandard fields should be shown.

Checklist

  • fixup! commits have been squashed
  • CI passes after rebase
  • CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

verified={contributor.is_verified}
/>
{visibleContributors.map(contributor => (
<ListItem
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched from using the FacilityDetailSidebarDetail here because the structure is different enough now to make it overly complex

<ShowOnly when={!hideTopDivider}>
<Divider className={classes.divider} />
<ListItem className={classes.item}>
<ShowOnly when={verified}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm showing the verified badge when an extended field is verified (presumably includes when the contributor is verified, based on upcoming changes). Do we want this, or should there be no badge for verified items?

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense that we visually indicate the status of these items. I like shoing the badge.

@@ -90,7 +90,7 @@ const getContent = ({
secondary: 'Claim this facility',
icon: <BadgeUnclaimed />,
style: {
background: 'rgb(9, 18, 50)',
background: 'rgb(61, 50, 138)',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating to new unclaimed header color per new wireframes

@@ -715,3 +715,37 @@ export const facilitySidebarActions = {
CLAIM_FACILITY: 'Claim this facility',
VIEW_ON_OAR: 'View on the Open Apparel Registry',
};

export const EXTENDED_FIELD_TYPES = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The constant array of extended field types includes anticipated fields which aren't yet included in the API

Copy link
Contributor

@jwalgran jwalgran left a comment

Choose a reason for hiding this comment

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

Nice work on the implementation! The code is clear and I was able to successfully work through the testing instructions without any issues.

I did see some things I did not expect, but I don't think they necessarily fall withing the scope of this PR. Please create follow up issues or add subtasks to one of our existing issues (listed under #1585) if appropriate.

  1. After claiming the facility, I would expect the values for the claimed fa facility to be promoted to the top of the list. In my testing it appeared at the end of the list

Screen Shot 2022-01-19 at 12 28 48 PM

  1. In my testing some addresses appear without a contributor name or date.

Screen Shot 2022-01-19 at 12 41 52 PM

  1. In my test I created an embedded map as "Service Provider A" but when I view a facility that was originally created under a different contributor ("Factory A"), I see that contributor's name. We need to avoid showing any other contributor names.

Screen Shot 2022-01-19 at 12 35 08 PM

<ShowOnly when={!hideTopDivider}>
<Divider className={classes.divider} />
<ListItem className={classes.item}>
<ShowOnly when={verified}>
Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense that we visually indicate the status of these items. I like shoing the badge.

@jwalgran jwalgran assigned TaiWilkin and unassigned jwalgran Jan 19, 2022
@TaiWilkin
Copy link
Contributor Author

TaiWilkin commented Jan 19, 2022

  1. After claiming the facility, I would expect the values for the claimed fa facility to be promoted to the top of the list. In my testing it appeared at the end of the list

This is caused by a) the prioritization of the name/address/location on the facility over the extended fields, and b) the combination/prioritization of the other_names/_addresses/_locations with the extended fields.

  1. In my testing some addresses appear without a contributor name or date.

This is caused by the combination of the other_names/_addresses/_locations with the extended fields (they don't include the contribution data).

  1. In my test I created an embedded map as "Service Provider A" but when I view a facility that was originally created under a different contributor ("Factory A"), I see that contributor's name. We need to avoid showing any other contributor names.

This is caused by the prioritization or and inclusion of contribution data for the facility object's name/address/locations.
These three issues will be fixed by updating the facility details and list APIs, specifically by:

  • Including the other_names/_addresses/_locations information with the extended_fields when serializing to allow proper server-side sorting and inclusion of contributor data
  • Prioritizing the name/address/location from an active/verified facility claim over the facility object's name/address/location when serializing (unless it is in embed mode and prefer_contributor_name is active, in which case prefer the contributor name)
  • Adjusting the created_from to hide other contributor information in embed mode
    These tasks have been added to #1570

Makes the facility details subsections expandable to show additional
content (details from other matches, claims, etc.) when not in embed
mode.

Adds extended field subsections (number of workers, native language
name, and so on) when values are present and the map is not in embed
mode.

Adds nonstandard/contributor field subsections when values are present
in embed mode.
@TaiWilkin
Copy link
Contributor Author

Thank you for reviewing!

@TaiWilkin TaiWilkin merged commit 468c038 into develop Jan 19, 2022
@TaiWilkin TaiWilkin deleted the tw/expand-fields branch January 19, 2022 21:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants