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

Support prefix & suffix for 3 widgets: string, number & boolean #6836

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ngdangtu-vn
Copy link

@ngdangtu-vn ngdangtu-vn commented Jul 7, 2023

This changes are about:

  • refactor syntax BooleanControl static properties
  • add prefix & suffix options
  • update widget control style to match with new options
  • update Boolean widget document

Summary

The update allows developers to display messages before (prefix) and after (suffix) the toggle. This design intends to provide a better information and look to users.

In my use case, I have a field to decide if I want to show a post in homepage. If I set "Show in homepage" string in label, the layout of input would be unbalance as the top small part is bigger than the main body. The toggle itself seems lonely as well. So if I can set "Visibility" in label option and "Show in homepage" in prefix option, it would be much sense.

Test plan

  1. Run yarn start
  2. Go to this path collections/kitchenSink/entries/a-big-entry-with-all-the-things
  3. Expect to see the exact Boolean input like this screenshot:
    image

Checklist

A picture of a cute animal (not mandatory but encouraged)

Be hold! The amazing meowman!

meow

@martinjagodic
Copy link
Member

@ngdangtu-vn I see why you would want this feature, but it seems very similar to hint, which exists on all widgets and can be used to explain a field in more detail. Is there a reason why you wouldn't use hint?

@ngdangtu-vn
Copy link
Author

ngdangtu-vn commented Aug 4, 2023

Is there a reason why you wouldn't use hint?

I wouldn't say I don't use hint. But my reason is simply:

  • Not wasting a whole line for only 1 toggle element
  • Hint text is small enough to be a hint only. Prefix/Suffix will plays nice the the role of important instruction.
  • In some messages, it looks better with prefix (or|and) suffix as well. I admit, my example wasn't very good. Maybe like this: Show author [x].

I even plan to do take this further to string and number widgets so dev can display fields like this [...]px. Cool right?

@martinjagodic
Copy link
Member

Extending this to string and number will be useful! Can you implement that as part of this PR and I will review/merge it all together?

@ngdangtu-vn
Copy link
Author

Update photo for recent commits

String Widget

Number Widget

Boolean Widget

@martinjagodic Could you review my code and the Widgets' UI? Thanks.

Copy link

@xaiki xaiki left a comment

Choose a reason for hiding this comment

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

fwiw, my 2c review

note: i'm not part of the decap team.

- { label: 'Title', name: 'title', widget: 'string' }
- { label: 'Boolean', name: 'boolean', widget: 'boolean', default: true }
- { label: 'Title', name: 'title', widget: 'string', prefix: 'This string:', suffix: 'is a title' }
- { label: 'Boolean', name: 'boolean', widget: 'boolean', prefix: 'Toggle this to switch on/off:', hint: 'Toggle this to switch on/off', default: true }
Copy link

Choose a reason for hiding this comment

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

having prefix and hint be the same string doesn't make for a good case of why prefix is needed.

@@ -117,11 +117,11 @@ collections: # A list of collections the CMS should be able to edit
display_fields: ['title', 'date']
search_fields: ['title', 'body']
value_field: 'title'
- { label: 'Title', name: 'title', widget: 'string' }
- { label: 'Boolean', name: 'boolean', widget: 'boolean', default: true }
- { label: 'Title', name: 'title', widget: 'string', prefix: 'This string:', suffix: 'is a title' }
Copy link

Choose a reason for hiding this comment

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

this suffix doesn't seem to make a lot of sense, is it a prefix ? is it a hint ? why would i want this ?

Copy link
Author

Choose a reason for hiding this comment

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

Nope, it's just for testing and demonstration. But you are welcome to improve it.

onBlur={setInactiveStyle}
Background={BooleanBackground}
/>
{suffix && (<span>&nbsp;{suffix}</span>)}
Copy link

Choose a reason for hiding this comment

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

i'm wondering if those spans should be classed so we can theme theme ?

Copy link
Author

Choose a reason for hiding this comment

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

We can go with div>span for both and add :first-of-type for prefix, similar goes to suffix. No need to go for class everywhere.


BooleanControl.defaultProps = {
value: false,
};
Copy link

Choose a reason for hiding this comment

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

any reason why these were moved as part of this specific PR ?

onChange={this.handleChange}
css={css`flex-grow: 1`}
/>
{suffix && (<span>&nbsp;{suffix}</span>)}
Copy link

Choose a reason for hiding this comment

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

wondering if onFocus and onBlur should go on the bigger div (classed {classNameWrapper})

onFocus={setActiveStyle}
onBlur={setInactiveStyle}
css={css`flex-grow: 1`}
/>
Copy link

Choose a reason for hiding this comment

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

ditto for onFocus/onBlur

Copy link

Choose a reason for hiding this comment

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

doc is required for number and string widgets too

@ngdangtu-vn
Copy link
Author

@xaiki

Thanks for your review but I'm not sure if they still need this PR. It kinda a dead PR already.

@martinjagodic
Copy link
Member

@xaiki thanks for your reviews

@ngdangtu-vn This PR is not dead, but I understand it sitting here for a long time now. We have quite a backlog and we want to add features based on the triage we decided upon. This one is not essential so it will probably wait a bit longer.

@ngdangtu-vn
Copy link
Author

@martinjagodic Ok it's cool, I thought the code won't be able to fit with the new Decap.

@xaiki So I will fix the PR using your advice later 'cause I'm bit busy recently & I need time to reload my memory :))

@xaiki
Copy link

xaiki commented Oct 5, 2023

@martinjagodic if reviews help I'm happy to look at other PRs, just point me to the top of the stack.

@martinjagodic
Copy link
Member

@xaiki I will happily assign you review requests. To talk more in-depth about what to do next, please join the contributing channel on our Discord server.

@martinjagodic
Copy link
Member

@ngdangtu-vn are you still interested in moving this forward? If yes, please address the comments and solve merge conflicts. Thanks!

@ngdangtu-vn
Copy link
Author

Ok, give me around 2 more days

@netlify
Copy link

netlify bot commented Oct 20, 2023

‼️ Deploy request for cms-demo rejected.

Name Link
🔨 Latest commit e9c3a9e

@ngdangtu-vn
Copy link
Author

I didn't close this? Hold on I'm reorganizing this... Maybe because I reset branch?

@martinjagodic
Copy link
Member

@ngdangtu-vn maybe. I guess you can just reopen and continue from there.

Copy link
Author

@ngdangtu-vn ngdangtu-vn left a comment

Choose a reason for hiding this comment

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

I'm working merging (copy & paste my old code with modify to fit the new). After that is working on doc example content. I would love to open for any example content suggestion.

@ngdangtu-vn ngdangtu-vn changed the title feat(BooleanControl.js): display messages along toggle Support prefix & suffix for 3 widgets: string, number & boolean Oct 21, 2023
Those 3 widgets are Boolean, Number, String

- update 3 widgets UI to display prefix & suffix
- refactor BooleanControl.js code structure
- update document for 3 widgets
- update demo config `dev-test/`
@ngdangtu-vn ngdangtu-vn reopened this Oct 21, 2023
@ngdangtu-vn ngdangtu-vn requested a review from xaiki October 21, 2023 16:10
Copy link

netlify bot commented Jan 8, 2024

Deploy Preview for decap-www ready!

Name Link
🔨 Latest commit 2d9cb89
🔍 Latest deploy log https://app.netlify.com/sites/decap-www/deploys/659c01c495853c00082824a5
😎 Deploy Preview https://deploy-preview-6836--decap-www.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ngdangtu-vn ngdangtu-vn requested a review from a team as a code owner March 29, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants