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

AntD: Customizable style on AutoField #404

Merged
merged 1 commit into from
Apr 6, 2018
Merged

AntD: Customizable style on AutoField #404

merged 1 commit into from
Apr 6, 2018

Conversation

webpn
Copy link
Contributor

@webpn webpn commented Apr 6, 2018

Making AutoField, and all other components enhanced via wrapField, customizable through a style prop.

@webpn webpn requested a review from radekmie as a code owner April 6, 2018 10:53
@codecov
Copy link

codecov bot commented Apr 6, 2018

Codecov Report

Merging #404 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #404   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         157    157           
  Lines        1396   1396           
=====================================
  Hits         1396   1396
Impacted Files Coverage Δ
packages/uniforms-antd/src/wrapField.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b525577...2990ed9. Read the comment docs.

@radekmie radekmie added the Type: Feature New features and feature requests label Apr 6, 2018
@radekmie
Copy link
Contributor

radekmie commented Apr 6, 2018

Thank you, @webpn!

@radekmie radekmie merged commit 10544db into vazco:master Apr 6, 2018
@webpn
Copy link
Contributor Author

webpn commented Apr 13, 2018

Thank you for accepting the pull request.
I don't know why, but when I pull the v1.24.0 update via npm i, the built code is not correctly updated.

Here is the built file uniforms-antd/wrapField.js loaded via npm i:

66.            required: required,
67.            style: { marginBottom: '12px' },
68.            validateStatus: error ? 'error' : undefined,

Here is the locally built file:

68.            required: required,
69.            style: style || { marginBottom: '12px' },
70.            validateStatus: error ? 'error' : undefined,

You can see that the code about thestyle prop is not correctly updated.

Did I forget something about the build on my pull req.?

@radekmie
Copy link
Contributor

radekmie commented Apr 13, 2018

@webpn: nope, my fault. I've published incorrect (old) version. Will do it in a moment.

I have to bind the build to the publish script...

EDIT: Done.

@webpn
Copy link
Contributor Author

webpn commented Apr 20, 2018

Yesterday I noticed an issue that affects my pull req: since I wanted to be able to customize the hardcoded style ({ marginBottom: '12px' }), I added a custom prop style on wrapField.

However, the style prop is applied also to the wrapped <Input /> element, so that, if you set a border in the style prop of the wrapper, you will get a border on the wrapper <Form.Item />, and the same border will be applied on the wrapped <Input /> element.

You can see an example on this CodeSandbox.

I didn't notice this issue before because I was testing only with margin style customization (setting margin: 0).

How can I fix this? I thought it would be better if the prop on the wrapper function was called wrapperStyle, rather than style.

@radekmie
Copy link
Contributor

Well, @webpn, it's a problem. You can submit a PR for it, but I have to think about it a bit more: it's a breaking change. Not a big deal, as there wasn't such prop for a long time and there is, which is also a breaking change, but it's a new feature, so semantic versioning allows it.

webpn added a commit to webpn/uniforms that referenced this pull request Apr 24, 2018
Since the "style" prop is affected by a serious issues (see vazco#404 (comment)), I updated the prop name into "wrapperStyle".
@webpn
Copy link
Contributor Author

webpn commented Apr 24, 2018

Ok, I created #414
I had the same concern about the fact that renaming the style prop could be considered a breaking change: that's why I preferred to discuss this before making any other PR.
But, since #404 introduced a bug (that is a potentially breaking change), maybe its fix won't be considered as a breaking change itself. #414 may be released in 1.24.3

webpn pushed a commit to webpn/uniforms that referenced this pull request Apr 24, 2018
Since the "style" prop is affected by a serious issues (see vazco#404 (comment)), I updated the prop name into "wrapperStyle".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features and feature requests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants