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

v4: Custom form inputs (checkbox, select, radio, file) #3047

Closed
5 tasks done
jquense opened this issue Mar 5, 2018 · 29 comments
Closed
5 tasks done

v4: Custom form inputs (checkbox, select, radio, file) #3047

jquense opened this issue Mar 5, 2018 · 29 comments

Comments

@jquense
Copy link
Member

jquense commented Mar 5, 2018

Read first! Contributing | Structuring a PR | Bootstrap docs

  • Checkbox
  • Radio
  • File input
  • Select
  • Range
@jquense jquense added this to To do in v4 support via automation Mar 5, 2018
@AnderssonChristian
Copy link

I can take this.

@AnderssonChristian
Copy link

AnderssonChristian commented Mar 6, 2018

@jquense Looking at the official bs4 docs, all examples have the input-label association done with the for and id attributes. The other method of wrapping each input inside a label isn't documented and doesn't seem to work (it used to in some examples in the alpha version I found).

Unless I am missing something, this is really a shame since it would be easier to implement that approach with React. Now we would need some creative way of generating a unique id. Ideas?

@jquense
Copy link
Member Author

jquense commented Mar 6, 2018

Do you have any specific examples that you mean? Checkboxes and Radios are semantically valid inside of a label but i don't know if the styles work.

@taion
Copy link
Member

taion commented Mar 6, 2018

I think we should either have a component that just corresponds to e.g. .form-check and handles rendering both the .form-check-input and the .form-check-label, or else have the component corresponding to .form-check work like the current <FormGroup> as a context provider that takes a controlId (or perhaps better inputId) prop.

@jquense
Copy link
Member Author

jquense commented Mar 6, 2018

I think in this case we mean the custom inputs, which need a different markup than the normal form-check and input case :/ https://getbootstrap.com/docs/4.0/components/forms/#checkboxes

I'd say in this cases we should just accept an id, and thread it to both the label and input

@jquense
Copy link
Member Author

jquense commented Mar 6, 2018

ugh i ddin't even realize the normal inputs do this as well, sorry @taion we're on the same page :P

@taion
Copy link
Member

taion commented Mar 6, 2018

i mean this is good, this means that checkboxes are more like normal form groups... just need a slightly different API is all... <FormCheck>, <FormCheck.Input>, and <FormCheck.Label> perhaps?

@jquense
Copy link
Member Author

jquense commented Mar 6, 2018

well sort of? the label is still in a different place. I've actually already started the normal form inputs @AnderssonChristian (not the custom style ones noted here tho!)

maybe I sohuld push that branch so folks can see what i was thinking?

@taion
Copy link
Member

taion commented Mar 6, 2018

We could do something cute with functional components, e.g.

const normalFormCheck = (
  <FormCheck checked label="blah" />
);

const reversedFormCheck = (
  <FormCheck checked label="blah">
    {({ input, label }) => <>{label} {input}</>}
  </FormCheck>
);

@AnderssonChristian
Copy link

AnderssonChristian commented Mar 6, 2018

@jquense right. Semantically correct but the styles don't seem to work.

I guess we could implement a required id prop, this would reflect the framework anyway... just kind of a bummer.

@taion dot notation has worked nicely before, I like that suggestion. Though I don't understand your last example.

@jquense
Copy link
Member Author

jquense commented Mar 7, 2018

@AnderssonChristian i'd say leave the id not required, we don't require for other inputs, but if it's there we can put it in the right place. We can revist tho when the PR hits!

@iamnapo iamnapo moved this from To do to In progress in v4 support Mar 24, 2018
@eladlevy
Copy link

Hey all, what's the status of this issue? It seems that Checkbox and Radio components are already implemented right? If something is missing i'm willing to take it.

@jquense
Copy link
Member Author

jquense commented Oct 13, 2018

The custom versions are still missing if you want to give that a shot

@eladlevy
Copy link

@jquense checkout my PR :)

@mmmikeal
Copy link

did someone break radio buttons? checkboxes seem fine to me, but a standard radio button is not showing any text when any text

im going to check if i have the latest react bootstrap version

@davecarlson
Copy link

Should checkbox and radio now be ticked as they were released in b3 ?

@taion taion closed this as completed Dec 14, 2018
v4 support automation moved this from In progress to Done Dec 14, 2018
@taion
Copy link
Member

taion commented Feb 23, 2019

Oops, I closed this too early.

@taion taion reopened this Feb 23, 2019
v4 support automation moved this from Done to To do Feb 23, 2019
@taion
Copy link
Member

taion commented Apr 4, 2019

It works just fine: https://react-bootstrap.github.io/components/forms/#forms-custom-checkboxes-and-radios. You most likely have the wrong CSS.

@ziyanwan
Copy link

ziyanwan commented Apr 4, 2019

It works just fine: https://react-bootstrap.github.io/components/forms/#forms-custom-checkboxes-and-radios. You most likely have the wrong CSS.

Sorry, I just realized that label and id are mandatory if using custom.

@JeromeDeLeon
Copy link
Contributor

How about Select? I think i has the same logic aside from its mandatory id and label. For File, it seems like the BS is using bs-custom-file-input.

@Zageron
Copy link

Zageron commented Sep 6, 2019

Hello, is there a recommended way to style the <Form.Control as="input" type="file" /> in typescript? I've found many javascript examples, but nothing for react.

In case it is relevant this is how I did it indirectly.

const FileInput: React.FC = ({
}) => {
  const inputFile = useRef(null);
  const onButtonClick = (): void => {
    inputFile.current.click();
  };

  return (
    <div className="FileInput">
      <input type="file" id="file" ref={inputFile} hidden />
      <Button variant="primary" onClick={onButtonClick}>
        Open file upload window
      </Button>
    </div>
  );
};```

@kosmiq
Copy link
Collaborator

kosmiq commented Mar 6, 2020

So I looked through this and have a few thoughts/questions.

First of the custom Select styling. Has there been any consensus on how such components should be created? It's currently simply a class added to whatever select you want to be custom and nothing more as far as I can tell (Bootstrap 4.4.1).

Would the correct way of doing it simply allow passing "custom" as a boolean to Form.Control when as is set to select? Or should we create a Form.Select component that accepts custom as a boolean instead?

I also noted that react-bootstrap currently is missing the Range control. This one I guess would best be created as Form.Range (mimicking Form.Check) and then allow for passing custom as a boolean for the custom styling?

The Form.Range component could be pretty simple if we do not allow all the options the spec currently supports, since all browsers (Firefox) does not support everything from the spec currently.

  • The Tick mark can be omitted since Firefox does not support it at all currently.
    • it can also be omitted since Bootstrap custom styling does not support them
  • Vertical orientation can be omitted since it relies on prefixes to function (and bootstrap does not mention it). Plus the fact that Bootstrap does not supply custom styling for the vertical version
  • Should we support list (datalist) for the range component (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range#list)?
    • Omitting it simplifies things. The Bootstrap docs does not mention it, and without supporting tick marks it has limited usage IMO. Defining min, max and steps covers everything you can accomplish with a list when omitting labels/marks.

If we can decide what would be the best way to add custom to the select dropdowns I might get the time to create a PR for it. After that I might as well get to doing a PR for the Range controller as well.

@taion
Copy link
Member

taion commented Mar 7, 2020

Looks like we should just add https://getbootstrap.com/docs/4.4/components/forms/#range ? It seems like it's just CSS...

@taion
Copy link
Member

taion commented Mar 7, 2020

Given that custom is just a prop on <Form.Check>, and that we otherwise model <select>s as <Form.Control>s, I think it'd make the most sense to accept a custom prop on <Form.Control> to render a custom <select>?

@jquense
Copy link
Member Author

jquense commented Mar 8, 2020

I probably have a branch with some of these that I abandoned because once you get into it the API is ugly and weird. That said happy to have someone else take a stab at it

@kosmiq
Copy link
Collaborator

kosmiq commented Mar 8, 2020

Looks like we should just add https://getbootstrap.com/docs/4.4/components/forms/#range ? It seems like it's just CSS...

Regarding range. Part of my proposal for creating form.range instead of adding it to form.control (with as="range") is that it also allows for min, max and step (not required, will be 0, 100 and 1 if not set). And I think that form.control might suddenly allow a bit too many props and feel a bit convoluted. Do you agree or should we simply add it to form.control and allow min, max and step there?

A PR has been created that

  • Adds Range
  • Adds custom boolean for range and select

@kosmiq
Copy link
Collaborator

kosmiq commented Mar 10, 2020

Alright. Here we go again. Sorry for a lengthy comment.

This time with the custom file input. I made a quick test in a react-bootstrap project I have up and running.

tldr;

  • The script linked in official bootstrap docs seem to be working fine
  • The custom file re-orders label and control plus it changes the class of the containing element (.form-group -> .custom-file)
  • This is probably enough to create a new component Form.File or similar?
    • Do we create Form.File and when custom is passed we render label and input in reverse order?
    • Or do we create Form.FileCustom and only handle the custom case in the component?
  • How do you think we should proceed on this?
  • What would be the best approach to init the bs-custom-file-input script?

------------- The long part -------------

I pasted the Bootstrap custom file markup

<div className="custom-file">
  <input type="file" className="custom-file-input" id="customFile" />
  <label className="custom-file-label" htmlFor="customFile">Choose file</label>
</div>

Then I installed the script referenced in their docs bs-custom-file-input (https://www.npmjs.com/package/bs-custom-file-input) and imported it into the project.

import bsCustomFileInput from 'bs-custom-file-input'

For the sake of testing I ran it on componentDidMount

componentDidMount() {
  bsCustomFileInput.init();
}

And everything seem to be working ok (filepicker dialog not visible, but I simply selected a file).
bootstrap-custom-file

But I guess we should talk a bit about this.

The markup for a custom file input differs a bit from a regular form input
Regular

<div class="form-group">
	<label for="exampleFormControlFile1">Example file input</label>
	<input type="file" class="form-control-file" id="exampleFormControlFile1">
</div>

Custom

<div class="custom-file">
	<input type="file" class="custom-file-input" id="customFile">
	<label class="custom-file-label" for="customFile">Choose file</label>
</div>

It replaces the form-group class with custom-file. In the docs it also changes the order of the input and label elements, but re-ordering these to be the same as for regular one seem to make no difference. That is until you try to use the data-browse attribute to set the content of the label:after thing. Re-ordering breaks this functionality.

Testing a little further, still init the script on componentDidMount() rather than in the component, both of these work, although with two differences. The first one gets 1rem margin-bottom and the second one does not (the custom-file class sets margin-bottom: 0)

<Form.Group controlId="customFile">
	<div className="custom-file">
		<Form.Control type="file" className="custom-file-input" />
		<Form.Label className="custom-file-label" data-browse="testtest">
			label
		</Form.Label>
	</div>
</Form.Group>
<Form.Group controlId="customFile" className="custom-file">
	<Form.Control type="file" className="custom-file-input" />
	<Form.Label className="custom-file-label">
		label
	</Form.Label>
</Form.Group>

Screenshot 2020-03-10 at 11 40 40

jquense added a commit that referenced this issue Mar 10, 2020
…input (#5034)

* #3047 add range and custom range input and custom prop for selects

#3047 add range and custom range input and custom prop for selects

* #3047 update FormControlSpec.js

#3047 Add tests

* #3047 update docs

* Update FormControl.js

clean up leftover commented code

* #3047 simplify custom error check

* Update FormControl.js

Re-add removed @types

* #3047 add types for custom

* Update src/FormControl.js

Co-Authored-By: Jimmy Jia <tesrin@gmail.com>

* Update src/FormControl.js

Co-Authored-By: Jason Quense <monastic.panic@gmail.com>

Co-authored-by: Tor Raswill <tor.raswill@dqc.se>
Co-authored-by: Jimmy Jia <tesrin@gmail.com>
Co-authored-by: Jason Quense <monastic.panic@gmail.com>
jquense pushed a commit that referenced this issue Mar 16, 2020
* #3047 add file input component

A file input component that also supports the custom prop.

Adds FormFile,  FormFileInput, FormFileLabel

Adds types

Updates docs

* #3047 remove bs-custom-file-input script

remove bs-custom-file-input script and import, remove useEffect since it is not used in FormFile for anything else.

* #3047 rename buttontext

rename buttontext to buttonText

* #3047 update docs

Update docs with how to customize the inner markup

* #3047 fix missed case for buttonText

* #3047 Update FileApi.js

Update FileApi.js with more comprehensive example

* #3047 add lang prop

The lang prop allows translating the Browse button text via SCSS as per the bootstrap documentation.

Add types and docs

* Update FileApi.js

fix conflicting Ids for elements

* #3047 Update forms.js

Update docs with
* recommended script for managing visible output of FormFile custom
* Better readability of FormFile docs when customizing the output

* #3047 Create FormFileSpec.js

Added test for FormFile

* #3047 removed buttonText prop

This commit removes the buttonText prop, the types for it and the test.

Instead it forwards the data-browse attribute to the underlying components.

Updates docs.

This is to be more uniform with TWBS.

* #3047 fix types

Added simple type test and added File to Form.d.ts

* Update simple.test.tsx

Fix lint/prettier error after removing ignore line from docs

* #3047 fix conflicting ID's

* #3047 FormFile docs

Updated to make sure that
* The example in the API for the "customizing" version" docs prints Input before Label since custom is not set
* Updates the description for the data-browse attribute
* Adds queries for the API docs for FormFile, FormFileInput and FormFileLabel

* Update FormFile.js

Add error for data-browse if custom is not set

* Add test for data-browse and typescript ref

* Added test for data-browse attribute
* Add FormFile to types export
* Add FormFile ref test to simple.test.tsx

* Clean up innerRefs

Per comments from @bpas247

* Update FormFile.js

Update the description printed in the API section

* Update FormFile.js

* add inputAs with desc
* rewrite as with new desc
* add type def to data-browse
* set form-file as default class instead of form-group when not setting custom
* use as like in other components to create wrapping div

* Rename FileButtonTextSCSS

rename one to incorrect since MacOS does not carer about casing in filenames

* Rename FileButtonTextScss

Rename it to its correct name, update imports and references

* Update FormFileSpec.js

Update tests to
* test changed class
* test inputAs
* test as with another element type

Co-authored-by: Tor Raswill <tor.raswill@dqc.se>
@kosmiq
Copy link
Collaborator

kosmiq commented Mar 17, 2020

Since #5036 and #5034 has been merged, should this be closed? :)

@jquense
Copy link
Member Author

jquense commented Mar 17, 2020

thanks for all the excellent work getting this finished @kosmiq !

@jquense jquense closed this as completed Mar 17, 2020
v4 support automation moved this from To do to Done Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v4 support
  
Done
Development

No branches or pull requests