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

[Content Manager] Add the new JSONInput from the DS to the CM #15606

Merged
merged 4 commits into from
Jan 31, 2023

Conversation

markkaylor
Copy link
Contributor

@markkaylor markkaylor commented Jan 27, 2023

What does it do?

  • Replaces the current json input component with the one migrated to the design system
  • Removes the old json input from the monorepo

Why is it needed?

  • To completely replace the old input

How to test it?

  • Open a content-type with a JSON field
  • You should see the new JSONInput
  • It should behave the same as the old input
    • The input should be editable
    • When incorrect JSON syntax is detected the errors should be highlighted
    • When submitting the form with incorrect JSON you should get an error message on the input
    • When the input is marked as required in the CTB and the form is submitted without any input there should be an error

Screenshot 2023-01-27 at 11 48 59

@markkaylor markkaylor self-assigned this Jan 27, 2023
@markkaylor markkaylor added pr: chore This PR contains chore tasks (cleanups, configs, tooling...) source: core:content-manager Source is core/content-manager package labels Jan 27, 2023
@markkaylor markkaylor added this to the 4.6.1 milestone Jan 27, 2023
Copy link
Contributor

@gu-stav gu-stav left a comment

Choose a reason for hiding this comment

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

I can't tell you how happy this makes me :D Thank you. Thank you. Thank you.

Would you mind adding a test to GenericInput to prove it renders the JSONInput for json fields?

Comment on lines 151 to 152
minHeight={pxToRem(253)}
maxHeight={pxToRem(506)}
Copy link
Contributor

Choose a reason for hiding this comment

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

@maevalienard Should we use that moment to align the JSON Input to our 4point grid? :) Would you have input on how we could improve these values?

Choose a reason for hiding this comment

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

We can make it:

minHeight={pxToRem(252)}
maxHeight={pxToRem(504)}

@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Base: 58.60% // Head: 59.12% // Increases project coverage by +0.51% 🎉

Coverage data is based on head (83a980f) compared to base (a3ff110).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15606      +/-   ##
==========================================
+ Coverage   58.60%   59.12%   +0.51%     
==========================================
  Files        1507     1502       -5     
  Lines       38696    38273     -423     
  Branches     7462     7357     -105     
==========================================
- Hits        22679    22627      -52     
+ Misses      13696    13378     -318     
+ Partials     2321     2268      -53     
Flag Coverage Δ
back 47.35% <ø> (+0.02%) ⬆️
front 67.12% <ø> (+1.00%) ⬆️
unit_back 47.35% <ø> (+0.02%) ⬆️
unit_front 67.12% <ø> (+1.00%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...min/src/content-manager/components/Inputs/index.js 15.92% <ø> (-0.74%) ⬇️
packages/core/data-transfer/src/engine/index.ts 90.81% <0.00%> (-0.28%) ⬇️
...e/data-transfer/src/file/providers/source/index.ts 34.61% <0.00%> (-0.04%) ⬇️
packages/core/data-transfer/src/utils/json.ts 100.00% <0.00%> (ø)
...dmin/admin/src/translations/languageNativeNames.js 100.00% <0.00%> (ø)
packages/core/utils/lib/pipe-async.js
packages/core/utils/lib/async.js 92.85% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

joshuaellis
joshuaellis previously approved these changes Jan 27, 2023
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Thanks Mark!! 🤩

Same as Gustav, but have a tick.

@maevalienard
Copy link

Looks great to me! I feel like we just have a small border-radius issue here:
Capture d’écran 2023-01-27 à 14 32 08
The background doesn't seem to fit the border properly in the corners :)

@markkaylor
Copy link
Contributor Author

Looks great to me! I feel like we just have a small border-radius issue here:

That's a bummer, I had noticed that a while back and thought I had fixed it. Guess not. I'll open a PR in the DS. Thanks!

Copy link
Contributor

@gu-stav gu-stav left a comment

Choose a reason for hiding this comment

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

Could you please remove all dependencies associated with the JSON input from the dependencies?

@markkaylor
Copy link
Contributor Author

Hey @gu-stav I don't think there are any dependencies I can remove since Editor still uses codemirror, I checked for anything else but let me know if you spotted something I missed

Copy link
Contributor

@gu-stav gu-stav left a comment

Choose a reason for hiding this comment

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

You are right - sorry. I didn't know we were using codemirror there too. Looks good to me then! 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: chore This PR contains chore tasks (cleanups, configs, tooling...) source: core:content-manager Source is core/content-manager package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants