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

Task/jenkins 39563 basic form comps #114

Merged
merged 20 commits into from Nov 18, 2016

Conversation

Projects
None yet
3 participants
@cliffmeyers
Copy link
Collaborator

commented Nov 14, 2016

Description

  • See JENKINS-39563.
  • This is a first pass at a couple of simple controls and some plumbing for forms and display validation errors. I thought it might be good to adopt of pattern of building basic controls (TextInput, RadioButtonGroup) that are simple, have minimal DOM clutter and are easy to compose. Then I created the FormElement wrapper that we can use for basic layout and error message presentation. Then I wrapped up TextInput and FormElement into a FormTextInput (not sure I like the name) with the idea that we could support common validation use cases via props, like "required" or "validatorRegex" or "validatorFunc".

Submitter checklist

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Appropriate unit or acceptance tests or explanation to why this change has no tests

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@reviewbybees

@cliffmeyers

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 14, 2016

cc @sophistifunk for feedback on the integration w/ FormElement and such. I want to nail down that API and make sure we're comfortable before I start building out other components and skinning everything up.

@cliffmeyers

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 15, 2016

I added a Checkbox component and also took a stab at how we might handle supporting the small, medium and large sizes in the style guide: 1. classes / mixins for each size of a component, typically not used directly unless there's a need to override in a one-off case, 2. applying those mixins to each component when they are nested within a container with a layout-small|medium|large class/mixin. We could apply those layouts as mixins to more semantic container names if we can come up with ones that make sense.

Cliff Meyers added some commits Nov 15, 2016

Cliff Meyers
[JENKINS-39563] refactor Favorite to compose Checkbox; clean up some …
…warnings caused by hyphenated svg attributes
@cliffmeyers

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 15, 2016

Refactored Favorite to just be a custom Checkbox.

@sophistifunk

This comment has been minimized.

Copy link
Collaborator

commented Nov 15, 2016

Just starting to look into this now. FWIW, I've found this to be a good starting point for organising component styles, and I've tried to stick pretty close to it for things I've built recently: https://github.com/suitcss/suit/blob/master/doc/naming-conventions.md

It seems pretty sensible on the whole, but it might have some blurry edges when we're creating components to be used in the nebulous world of future, third part code :)

position: absolute;
width: 1px;
}

This comment has been minimized.

Copy link
@sophistifunk

sophistifunk Nov 15, 2016

Collaborator

How does this work with keyboard tab-focus? n/m :D

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Nov 15, 2016

Author Collaborator

It does work. That's really the reason for those ugly combination of flip / height / margin junk - so you can skin the other element but let the browser still recognize checkbox by not hiding it entirely.

}
}

input[type="checkbox"]:focus + .Checkbox-indicator {

This comment has been minimized.

Copy link
@sophistifunk

sophistifunk Nov 15, 2016

Collaborator

Nice 👍

@@ -0,0 +1,25 @@
.radio-button-group {

.radio-button-wrapper {

This comment has been minimized.

Copy link
@sophistifunk

sophistifunk Nov 15, 2016

Collaborator

Why not just put these -wrapper styles on the parent div and ditch the extra wrapper, since there's no styles on the outer .radio-button-group?

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Nov 16, 2016

Author Collaborator

Fixed in commit 5a7eafd

.layout-small {
font-size: 12px;

.Checkbox {

This comment has been minimized.

Copy link
@sophistifunk

sophistifunk Nov 15, 2016

Collaborator

Could these be expressed within the Checkbox styles as

.Checkbox {
    // ...regular checkbox styles
    .layout-small & {
        // ... small checkbox styles
    }
}

To keep all the checkbox styles in one place?

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Nov 15, 2016

Author Collaborator

Right on, totally forgot about that syntax. You like the convention of layout-small|medium|large then I take it?

This comment has been minimized.

Copy link
@sophistifunk

sophistifunk Nov 15, 2016

Collaborator

Seems OK, we can try it on for size at least :)

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Nov 16, 2016

Author Collaborator

Fixed in commit 2765c52

<input type="checkbox"
onChange={this.toggle.bind(this)}
checked={this.state.checked} />

<svg className="star-icon" width="288" height="24" viewBox="0 0 288 24" xmlns="http://www.w3.org/2000/svg">

This comment has been minimized.

Copy link
@sophistifunk

sophistifunk Nov 15, 2016

Collaborator

We should probably extract complex SVG shapes (more than a handful of lines) into separate components IMO, it makes them easier to use, and makes the render() of this compound component easier to grok. Doesn't need to be a fully polished exported component until we need it, but I find even a file-local component makes it easier to read.

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Nov 15, 2016

Author Collaborator

So define a stateless function component FavoriteStar within the file and then drop it Favorite's render method? Sure, I can absolutely do that.

This comment has been minimized.

Copy link
@cliffmeyers

cliffmeyers Nov 16, 2016

Author Collaborator

Fixed in dd8d21d

Cliff Meyers
[JENKINS-39563] remove excess DOM now that we've verified the text al…
…ign issue in IE11 is not flexbox related (and possible flexbox hacks won't help us anyways)
@sophistifunk

This comment has been minimized.

Copy link
Collaborator

commented Nov 15, 2016

Looks good, minor nitpicks aside 😁

bee

@cliffmeyers

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 15, 2016

@sophistifunk did you have any feedback on the FormElement and the concept of wrapping up a TextInput + FormElement into a more feature-rich control? If not, that's cool, I'll keep moving in that direction.

@sophistifunk

This comment has been minimized.

Copy link
Collaborator

commented Nov 15, 2016

I like having a wrapping component, but I'd like to see the <label for="fooBar"/>...<input id="fooBar"/> pairing. But this can complicate checkboxes / radios, which have their own label (which should have preference in for="") as well.

Cliff Meyers added some commits Nov 16, 2016

Cliff Meyers
[JENKINS-39563] consolidate the logic for text controls into "TextCon…
…trol" and let "TextInput" and "TextArea" compose it; better skinning for both controls
Cliff Meyers
Cliff Meyers
Cliff Meyers
[JENKINS-39563] fix up layouts and apply styles to Checkbox in clean …
…fashion; some more storybooks for Checkbox
Cliff Meyers
@cliffmeyers

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 16, 2016

@sophistifunk I have taken this about as far as I would like in this PR, otherwise it is just going to become an unwieldy monster :) Perhaps we can discuss the label semantics when we have a few mins? There are some considerations (like ID collisions) for label for='' - maybe we want to evaluate the trade-offs. I had thought a long time ago I confirmed that nesting content in label is legit per W3C spec but will need to dig to look.

@sophistifunk

This comment has been minimized.

Copy link
Collaborator

commented Nov 16, 2016

Works for me mate.

@cliffmeyers

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 16, 2016

@sophistifunk sweet. Maybe you want to have a peek at the new commits - might be the easiest way for you to cut through the clutter. Thanks!

@sophistifunk

This comment has been minimized.

Copy link
Collaborator

commented Nov 16, 2016

Had a quick look, it all looks sensible to me. The 🐝 stands :)

@reviewbybees

This comment has been minimized.

Copy link
Collaborator

commented Nov 18, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

Cliff Meyers added some commits Nov 18, 2016

Cliff Meyers
Merge branch 'master' into task/JENKINS-39563-basic-form-comps
# Conflicts:
#	src/js/components/favorite/Favorite.jsx
Cliff Meyers
[JENKINS-39563] delint; export new components that are ready from ind…
…ex.js; disable storybook that isn't quite ready yet

@cliffmeyers cliffmeyers merged commit e8659a4 into master Nov 18, 2016

1 check passed

continuous-integration/jenkins/branch This commit looks good
Details

@cliffmeyers cliffmeyers deleted the task/JENKINS-39563-basic-form-comps branch Nov 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.