Add select component by JosephusPaye · Pull Request #227 · NUbots/NUsight2

JosephusPaye · 2019-02-04T05:28:11Z

This PR adds a generic single select component. We might expand it to allow multi-select later.

Used in the robot selector for #208.

Storybook

https://nusight-pr-227.herokuapp.com/storybook/?selectedKind=components.select&selectedStory=renders%20basic&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybook%2Factions%2Factions-panel

BrendanAnnable

Nice!

Gave it a quick skim, I'll have another deeper look soon 🙂

BrendanAnnable · 2019-02-04T06:46:10Z

src/client/components/select/view.tsx

+    )
+
+    return (
+      <OutsideClickHandler onOutsideClick={() => this.close()}>


This render method currently creates functions every time it executes, this is generally to be avoided for performance reasons, ideally it should create none.

Aside: We should probably add the jsx-no-lambda tslint rule from https://github.com/palantir/tslint-react to make it easy to spot 🙂

Event handlers can be fixed by using the @action.bound annotation instead of just @action, then you can just refer to this.close here instead.

The child items (e.g. which use () => this.onSelect(option)) can be solved by splitting out each item into its own react component, such that each item has its own @action.bound onSelect() {} method that can access its own props. I can give an example if that makes no sense.

That makes sense 👍. I'll update to use @action.bound, and extract a standalone select option component.

BrendanAnnable · 2019-02-04T07:22:10Z

src/client/components/select/view.tsx

+  private removeListeners?: () => void
+
+  componentDidMount() {
+    const onKeydown = (event: KeyboardEvent) => this.onDocumentKeydown(event)


Just use property syntax on the method, then it doesn't need to be rebound here. e.g.

private readonly onDocumentKeydown = (event: KeyboardEvent) => { // ... }

BrendanAnnable · 2019-02-04T07:22:41Z

src/client/components/select/view.tsx

+@observer
+export class Select extends React.Component<SelectProps> {
+  @observable
+  private isOpen: boolean = false


Do you think we will ever want to control the open-state of this component from outside?

Probably.

I couldn't think of a nice way to allow it to be controlled from outside (via a prop) while still being able to open/close it from inside, since changing the prop would violate the one-way data flow from parent to child.

Maybe there's a pattern I'm missing here.

Happy to leave it for now, but if we ever do, the pattern is generally to make 2 components:

The first is a pure/dumb component, no internal state, only reacts to its props. Basically just visual styling only.

The seconds wraps/controls the first, it manages all the state (it has all the observables and actions), and just delegates to the first.

https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0

BrendanAnnable · 2019-02-04T07:25:47Z

src/client/components/select/stories/select.stories.tsx

+  onChange: action('onChange'),
+}
+
+const container = { maxWidth: '320px', fontFamily: 'Arial, sans-serif' }


Might as well just make this a component

const Container = ({ children }: { children: any }) => { <div style={{ maxWidth: '320px', fontFamily: 'Arial, sans-serif' }}> {children} </div> }

Each story can then use

<Container> <Select props/> </Container>

Also fix lint errors from the new rule

BrendanAnnable · 2019-02-05T08:54:34Z

Could we add an interactive story as well? Like the switch has 🙂

BrendanAnnable

Some suggestions, but LGTM 🎉

BrendanAnnable · 2019-02-05T23:19:58Z

src/client/components/select/view.tsx

+@observer
+export class Select extends React.Component<SelectProps> {
+  @observable
+  private isOpen: boolean = false


Happy to leave it for now, but if we ever do, the pattern is generally to make 2 components:

The first is a pure/dumb component, no internal state, only reacts to its props. Basically just visual styling only.

The seconds wraps/controls the first, it manages all the state (it has all the observables and actions), and just delegates to the first.

https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0

BrendanAnnable · 2019-02-05T23:20:31Z

src/client/components/select/view.tsx

+  }
+
+  private readonly onDocumentKeydown = (event: KeyboardEvent) => {
+    if (this.isOpen && event.keyCode === KeyCode.Escape) {


FYI you can now just use event.key === 'Escape'

http://keycode.info/ is handy.

BrendanAnnable · 2019-02-05T23:21:02Z

tslint.json

+    "jsx-use-translation-function": false,
+    "jsx-self-close": false,
+    "jsx-space-before-trailing-slash": false,
+    "jsx-wrap-multiline": false


I haven't really reviewed these settings, but can we revisit if they ever become a problem!

I disabled those rules since I only wanted the jsx-no-lambda, but they're all enabled by default when you extend the preset.

Enabling them might require some fixes across the codebase.

BrendanAnnable · 2019-02-05T23:26:13Z

src/client/components/select/view.tsx

+
+export type SelectProps = {
+  className?: string
+  dropdownMenuClassName?: string


Passing classes directly into components is generally an anti-pattern. It breaks encapsulation.

It forces the consumer to provide a class which must understand the internal DOM structure of the component, which means you can't refactor the component without potentially breaking consumers who put weird CSS properties all over the place.

It's better for the component to remain responsible for styling itself, and allow consumers to set various "styles" via a constrained set of props.

I'm not sure what we plan to use this for, but for example instead of passing a className to increase the padding of an element, it would be better to instead pass in paddingStyle: 'compact' | 'wide' prop and let the component own the CSS styling.

We need it to style the dropdown (width, shadows, etc), since the basic dropdown component only sets the position.

Maybe we can move that there.

Removed as it wasn't needed.

Also removed the dropdownMenuClassName prop on the Dropdown component, replaced with two new props:

isFullwidth: boolean: which adds a class for width: 100%`

dropdownPosition: 'left' | 'right': adds a class which styles right-aligned dropdowns, used by the current robot selector

Instead use other props which handle the styling internally. Also remove KeyCode enum

JosephusPaye · 2019-02-07T03:38:40Z

@BrendanAnnable Added an interactive story and made other suggested changes. I think it's ready to merge.

JosephusPaye added 2 commits February 4, 2019 16:08

Add Select with stories

821129d

Move classname props to top

45d8140

BrendanAnnable temporarily deployed to nusight-pr-227 February 4, 2019 05:28 Inactive

JosephusPaye requested a review from BrendanAnnable February 4, 2019 05:45

BrendanAnnable reviewed Feb 4, 2019

View reviewed changes

Add feedback changes

1b17d19

BrendanAnnable temporarily deployed to nusight-pr-227 February 5, 2019 05:25 Inactive

Add tslint-react rules and enable jsx-no-lambda

4117760

Also fix lint errors from the new rule

BrendanAnnable had a problem deploying to nusight-pr-227 February 5, 2019 06:22 Failure

Fix tslint-react version in yarn.lock

9765e42

BrendanAnnable temporarily deployed to nusight-pr-227 February 5, 2019 06:33 Inactive

BrendanAnnable previously approved these changes Feb 5, 2019

View reviewed changes

JosephusPaye added 2 commits February 7, 2019 14:28

Remove dropdownMenuClassName prop on Select and Dropdown

a83cf9c

Instead use other props which handle the styling internally. Also remove KeyCode enum

Add interactive story

ad51774

JosephusPaye dismissed BrendanAnnable’s stale review via ad51774 February 7, 2019 03:35

BrendanAnnable temporarily deployed to nusight-pr-227 February 7, 2019 03:35 Inactive

BrendanAnnable approved these changes Feb 7, 2019

View reviewed changes

Merge branch 'master' into paye/select-component

5169d14

BrendanAnnable deployed to nusight-pr-227 February 7, 2019 03:59 View deployment

JosephusPaye merged commit 0de9e6c into master Feb 7, 2019

JosephusPaye mentioned this pull request Feb 7, 2019

Fix routing #233

Merged

TrentHouliston deleted the paye/select-component branch February 7, 2019 08:00

Conversation

JosephusPaye commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrendanAnnable left a comment

Choose a reason for hiding this comment

Uh oh!

BrendanAnnable Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JosephusPaye Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrendanAnnable commented Feb 5, 2019

Uh oh!

BrendanAnnable left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrendanAnnable Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JosephusPaye commented Feb 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JosephusPaye commented Feb 4, 2019 •

edited

Loading

BrendanAnnable Feb 4, 2019 •

edited

Loading

JosephusPaye Feb 4, 2019 •

edited

Loading

BrendanAnnable Feb 5, 2019 •

edited

Loading