-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
base: main
Are you sure you want to change the base?
Conversation
@ngdangtu-vn I see why you would want this feature, but it seems very similar to |
I wouldn't say I don't use hint. But my reason is simply:
I even plan to do take this further to |
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? |
Update photo for recent commits @martinjagodic Could you review my code and the Widgets' UI? Thanks. |
There was a problem hiding this 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.
dev-test/config.yml
Outdated
- { 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 } |
There was a problem hiding this comment.
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.
dev-test/config.yml
Outdated
@@ -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' } |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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> {suffix}</span>)} |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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, | ||
}; |
There was a problem hiding this comment.
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> {suffix}</span>)} |
There was a problem hiding this comment.
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`} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto for onFocus/onBlur
There was a problem hiding this comment.
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
Thanks for your review but I'm not sure if they still need this PR. It kinda a dead PR already. |
@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. |
@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 :)) |
@martinjagodic if reviews help I'm happy to look at other PRs, just point me to the top of the stack. |
@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. |
@ngdangtu-vn are you still interested in moving this forward? If yes, please address the comments and solve merge conflicts. Thanks! |
Ok, give me around 2 more days |
e1d1e4a
to
f646317
Compare
|
Name | Link |
---|---|
🔨 Latest commit | e9c3a9e |
I didn't close this? Hold on I'm reorganizing this... Maybe because I reset branch? |
@ngdangtu-vn maybe. I guess you can just reopen and continue from there. |
There was a problem hiding this 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.
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/`
✅ Deploy Preview for decap-www ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This changes are about:
BooleanControl
static propertiesprefix
&suffix
optionsSummary
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" inprefix
option, it would be much sense.Test plan
yarn start
collections/kitchenSink/entries/a-big-entry-with-all-the-things
Checklist
A picture of a cute animal (not mandatory but encouraged)
Be hold! The amazing meowman!