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

Improve field label from key heuristics #5403

Merged
merged 14 commits into from
Apr 8, 2021
Merged

Conversation

gwyneplaine
Copy link
Contributor

@gwyneplaine gwyneplaine commented Apr 8, 2021

Description

Field labels were previously using inflection to coerce the field key into a human readable label.
This caused incongruity with acronyms in particular. To address this we've done the following:

  • Remove inflection from the fields package
  • Moved the core logic in the keyToLabel utility in @keystone-next/keystone-legacy to @keystone-next/utils-legacy
  • Made minor adjustments to the logic. **
  • Replace the invocation of inflection in the fields implementation with the new humanize utility.

**The previous logic in keyToLabel would turn the following string standardHTMLThing to Standard HTMLThing
This is not consistent with the asserted behaviour, we've augmented this logic now to ensure the transformation is as follows:
standardHTMLThing --> Standard HTML Thing

Before:
Screen Shot 2021-04-08 at 10 59 51 am

After:
Screen Shot 2021-04-08 at 12 24 53 pm

I'm not really sure if this should be considered a breaking change or not, as I'm unclear if the label also affects non ui specific operations. (for example the returned value of keyToLabel is used both in pluralize and singularize in the list implementation)
perhaps @timleslie or @mitchellhamilton may have a semver suggestion here.

----- Edit ------
Removing the logic changes to keyToLabel / humanize, as this would be a major breaking change.
The field label logic should still be a drastic improvement from before.

@changeset-bot
Copy link

changeset-bot bot commented Apr 8, 2021

🦋 Changeset detected

Latest commit: e561094

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@keystone-next/fields Patch
@keystone-next/utils-legacy Minor
@keystone-next/keystone-legacy Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Apr 8, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/keystonejs/keystone-next-docs/9j7YxUBNR6jWWcgCPN3qf5VEndXE
✅ Preview: https://keystone-next-docs-git-key-354-improve-heuristics-keystonejs.vercel.app

@gwyneplaine gwyneplaine force-pushed the KEY-354/improve-heuristics branch 3 times, most recently from e78e0ff to 6abda2a Compare April 8, 2021 02:31
@vercel vercel bot temporarily deployed to Preview April 8, 2021 02:32 Inactive
@emmatown
Copy link
Member

emmatown commented Apr 8, 2021

Yeah, this is a major because the label for lists that comes from keyToLabel is then passed through pluralize and then some more things happen to it and it ends up in the GraphQL API.

@vercel vercel bot temporarily deployed to Preview April 8, 2021 02:42 Inactive
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 8, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e561094:

Sandbox Source
@keystone-next/example-sandbox Configuration

@gwyneplaine
Copy link
Contributor Author

Yeah, this is a major because the label for lists that comes from keyToLabel is then passed through pluralize and then some more things happen to it and it ends up in the GraphQL API.

@mitchellhamilton mm, releasing a major for a slightly more correct label utility seems bad to me.
I'll revert the logical changes made for this, and it should still satisfy the field bug correction.

At which point we can probably release this as a patch, provided the label in the packages/fields/src/Implementation.js is isolated to just UI concerns. 🤞

@vercel vercel bot temporarily deployed to Preview April 8, 2021 04:15 Inactive
@vercel vercel bot temporarily deployed to Preview April 8, 2021 04:22 Inactive
@gwyneplaine gwyneplaine requested a review from a team April 8, 2021 04:23
Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Needs a changeset

not at all, missed this when i removed logic :D

Co-authored-by: Tim Leslie <timl@thinkmill.com.au>
@vercel vercel bot temporarily deployed to Preview April 8, 2021 04:53 Inactive
@vercel vercel bot temporarily deployed to Preview April 8, 2021 04:56 Inactive
@vercel vercel bot temporarily deployed to Preview April 8, 2021 05:03 Inactive
Copy link
Contributor

@timleslie timleslie left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gwyneplaine gwyneplaine enabled auto-merge (squash) April 8, 2021 05:12
@gwyneplaine gwyneplaine merged commit d0adec5 into master Apr 8, 2021
@gwyneplaine gwyneplaine deleted the KEY-354/improve-heuristics branch April 8, 2021 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants