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

Use sectionPrefix when using an empty name on a Field inside a FormSection #4118

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

Use sectionPrefix when using an empty name on a Field inside a FormSection #4118

wants to merge 5 commits into from

Conversation

SBRK
Copy link
Contributor

@SBRK SBRK commented Jul 4, 2018

I found myself wanting a Field which, inside a FormSection would use the name passed to the FormSection. For that, I tried passing name='', to no avail.

This is useful if, like in my case, you want a text field to edit your whole section as JSON, while also having other fields editing different fields of the section:

<FormSection name='foo'>
 <JSONField name=''/> // Will edit foo as json
 <InputField name='title'/> // Will edit foo.title
 [...]
</FormSection>

@SBRK SBRK changed the title Feature/prefixname empty name Use sectionPrefix when using an empty name in a FormSection Jul 4, 2018
@SBRK SBRK changed the title Use sectionPrefix when using an empty name in a FormSection Use sectionPrefix when using an empty name on a Field inside a FormSection Jul 4, 2018
@codecov
Copy link

codecov bot commented Jul 4, 2018

Codecov Report

Merging #4118 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4118   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          72      72           
  Lines        1666    1666           
======================================
  Hits         1666    1666
Impacted Files Coverage Δ
src/util/prefixName.js 100% <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 165f7ff...7110fc5. Read the comment docs.

Copy link
Collaborator

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

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

Must include some tests.

Improving the docs would be good, but not a must IMO.

@danielrob
Copy link
Collaborator

I'd worry about the 0 case. Not that numerical names particularly work anyway, but I think some people have used them to e.g. access array indexes. Might be good to explicitly check name === '' just in case.

@codecov-io
Copy link

codecov-io commented Jul 5, 2018

Codecov Report

Merging #4118 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4118   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          74      72    -2     
  Lines        1694    1669   -25     
======================================
- Hits         1694    1669   -25
Impacted Files Coverage Δ
src/util/prefixName.js 100% <100%> (ø) ⬆️
src/immutable.js 100% <0%> (ø) ⬆️
src/ConnectedField.js 100% <0%> (ø) ⬆️
src/Form.js 100% <0%> (ø) ⬆️
src/createReducer.js 100% <0%> (ø) ⬆️
src/util/isHotReloading.js 100% <0%> (ø) ⬆️
src/structure/immutable/index.js 100% <0%> (ø) ⬆️
src/index.js 100% <0%> (ø) ⬆️
src/FormSection.js 100% <0%> (ø) ⬆️
src/structure/immutable/deepEqual.js 100% <0%> (ø) ⬆️
... and 14 more

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 720c9d0...742225d. Read the comment docs.

@SBRK
Copy link
Contributor Author

SBRK commented Jul 5, 2018

@danielrob @gustavohenke I added the tests and handled the case name = 0

@erikras
Copy link
Member

erikras commented Jul 18, 2018

I wonder if this should be documented somehow? Maybe a paragraph under FormSection?

@erikras
Copy link
Member

erikras commented Dec 11, 2018

Sorry, but you'll need to refactor this with the new context API.

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.

None yet

5 participants