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

feat(ui-concerto): Improve rendering for relationship values #164

Merged
merged 5 commits into from
Aug 13, 2020

Conversation

mttrbrts
Copy link
Member

@mttrbrts mttrbrts commented Aug 12, 2020

Summary

This PR improves the formatting for relationship fields to better support relationship URIs (e.g. resource:test.AnAsset#49253). The internal URI format is difficult to understand for a novice user and the current field doesn't enforce the URI format when editing, this can easily lead to mistakes.

Changes

  • Relationship fields and relationship array fields now use the new ConcertoRelationship component which understands the relationship URI. This is instead of the ConcertoInput component which is used elsewhere.
  • The new ConcertoRelationship component now hides the URI protocol and namespace from the path. The Type is shown in a label, and only the identifier value is editable.
  • Also, this PR changes the styling of the array Add and Remove buttons. These buttons also include aria labels to improve accessibility.

Flags

Screenshots or Video

image

Author Checklist

  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to master from fork:branchname
  • Manual accessibility test performed
    • Keyboard-only access, including forms
    • Contrast at least WCAG Level A
    • Appropriate labels, alt text, and instructions

Signed-off-by: Matt Roberts <code@rbrts.uk>
Copy link
Contributor

@dselman dselman left a comment

Choose a reason for hiding this comment

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

  1. The right arrow doesn't render on Safari.
  2. The event that is getting fired doesn't seem to include the ID for the relationship

image

@Michael-Grover
Copy link

What does the arrow and tag mean? I don't think I've seen this in a form UI before.

What did this UI look like before the change?

image

@mttrbrts
Copy link
Member Author

What does the arrow and tag mean? I don't think I've seen this in a form UI before.

What did this UI look like before the change?

image

Previously this just appeared as a standard input field. I'm trying to find a way to distinguish between values and relationships (https://docs.accordproject.org/docs/model-relationships.html).
This is required because without it we end up displaying ugly values in the field, e.g. resource:test.AnAsset#49253 when the only thing that I should be able to change is 49253

In the latest iteration, I've added a popup to give more explanation
image

…ference

Signed-off-by: Matt Roberts <code@rbrts.uk>
@mttrbrts mttrbrts force-pushed the mr-ui-concerto-relationships branch from 434a245 to 6eb8819 Compare August 12, 2020 15:04
Signed-off-by: Matt Roberts <code@rbrts.uk>
@mttrbrts
Copy link
Member Author

  1. The right arrow doesn't render on Safari.
  2. The event that is getting fired doesn't seem to include the ID for the relationship
image

I've fixed 2, I believe that this was an instance of bad URI parsing. I'm now using the concerto Relationship.toURI/fromURI functions.

  1. Appears to be a preexisting issue related to rollup. I can only replicate in netlify.

@mttrbrts mttrbrts marked this pull request as ready for review August 12, 2020 20:05
Signed-off-by: Matt Roberts <code@rbrts.uk>
Copy link
Member

@DianaLease DianaLease left a comment

Choose a reason for hiding this comment

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

@jeromesimeon
Copy link
Member

As a developer, I really love the arrow and resource syntax under the field. I think it looks very nice.

Signed-off-by: Matt Roberts <code@rbrts.uk>
Copy link
Member

@jeromesimeon jeromesimeon left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -58,6 +58,7 @@
"scripts": {
"storybook": "start-storybook -p 9009 -s public",
"build-storybook": "NODE_OPTIONS='--max-old-space-size=4096' build-storybook -s public --quiet",
"build-storybook:watch": "NODE_OPTIONS='--max-old-space-size=4096' build-storybook -w -s public --quiet",
Copy link
Member

Choose a reason for hiding this comment

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

The README says to use npm run storybook.. does this do something different?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is helpful when developing to have the storybook auto-rebuild when the code changes. The README is correct, but personally I prefer to work like this.

@mttrbrts mttrbrts merged commit f7be2a0 into accordproject:master Aug 13, 2020
@mttrbrts mttrbrts deleted the mr-ui-concerto-relationships branch August 13, 2020 14:01
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Package: UI Concerto 💾 Type: Enhancement ✨ Improvement to process or efficiency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants