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

docs(TableExampleSortable): pass in null when that column isn't sorted #1737

Merged
merged 1 commit into from
Jun 6, 2017

Conversation

jayphelps
Copy link
Contributor

@jayphelps jayphelps commented Jun 5, 2017

Docs demo were incorrect, this PR fixes them

// before
<Table.HeaderCell sorted={column === 'name' && direction} />
// after
<Table.HeaderCell sorted={column === 'name' ? direction : null} />

BELOW IS THE ORIGINAL POST WHICH IS NO LONGER APPLICABLE

Right now the <Table sortable /> demo in the docs uses this JSX logic:

<Table.HeaderCell sorted={column === 'name' && direction} />

This provides the value false when that column is not selected, however it actually causes a PropTypes validation error in React dev mode. It wasn't noticed because the demo site runs in production mode, where PropTypes are not checked.

https://jsfiddle.net/cq3tfzmf/

image


Because this pattern is indeed very useful, this PR adjusts the proptypes and TS to accept false as a valid option. useValueAndKey(false, 'sorted') seemingly correctly handles this case by ignoring it and not rendering false sorted, however the tests currently fail because it's not clear to me how to safely amend them.

I've tracked down the offending helper, classNamePropValueBeforePropName but I'm hesitant to change it ignore false values without knowing the reprecusions. Will need maintainer guidance, if this PR is otherwise acceptable.


Alternatives

The alternative I can think of is returning null|undefined which appears to work around it. We would then need to amend the docs to use this.

<Table.HeaderCell sorted={(column === 'name' && direction) || null} />

Commit message marked as a feature since the API docs themselves didn't ever claim this was supported, only the demo implied it was. If your interpretation is that this was a "bug", obv lmk and I'll change.

@levithomason
Copy link
Member

I think if we did this, we should update all similar props to accept false. I think rather than mixing types for extra shorthand convenience here, I'd propose that we instead update the docs to ternary notation:

<Table.HeaderCell sorted={column === 'name' ? direction : null} />

My thinking is that we have several props which do take a string value or a boolean but they take both true and false and I wouldn't want to confuse the sorted prop which has no boolean alternative:

<Message icon>           // boolean prop
  <Icon name='info' />
</Message>

<Message icon='info' />  // shorthand value prop

I'm open to being convinced otherwise.

@jayphelps
Copy link
Contributor Author

@levithomason I like the ternary solution. Silly that I didn't think of that. 👍 🎉 Will update my PR to just update the demo in the docs

@codecov-io
Copy link

codecov-io commented Jun 5, 2017

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1737   +/-   ##
=======================================
  Coverage   99.75%   99.75%           
=======================================
  Files         142      142           
  Lines        2468     2468           
=======================================
  Hits         2462     2462           
  Misses          6        6

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 dbc0bd0...52da53c. Read the comment docs.

@jayphelps jayphelps changed the title feat(Table.HeaderCell): support sorted={false} for simplified JSX logic docs(TableExampleSortable): pass in null when that column isn't sorted Jun 5, 2017
@jayphelps
Copy link
Contributor Author

@levithomason PR updated to just change the demo

@levithomason
Copy link
Member

Thanks much, @jayphelps.

@levithomason levithomason merged commit 62b0fd6 into Semantic-Org:master Jun 6, 2017
aabustamante added a commit to aabustamante/Semantic-UI-React that referenced this pull request Jun 15, 2017
* docs(Button): remove redundant prop in Vertical Group example (Semantic-Org#1699)

* feat(typescript): Export generics types (Semantic-Org#1698)

* docs(Dropdown): fix world icon in search example (Semantic-Org#1695)

* Fix MenuExampleText

* add world icon to Dropdown example

* rename 'languageOptions' to 'languages'

* rename  languages   to languageOptions and separate line into two

* separate line of code in two

* separate props in different lines

* remove trailing spaces

* docs(Introduction): fix declarative example (Semantic-Org#1704)

* feat(Item): add unstackable prop to ItemGroup (Semantic-Org#1706)

* feat(Item): add unstackable prop to ItemGroup

* docs(Item): add example for unstackable

* chore(package): commit package-lock.json

* 0.68.4

* docs(changelog): update changelog [ci skip]

* docs(Icon): fix selector for input (Semantic-Org#1714)

* chore(package): update require-dir to version 0.3.2 (Semantic-Org#1721)

https://greenkeeper.io/

* docs(ItemExampleFloated): your description (Semantic-Org#1719)

Fixes error in docs. "AS" is not being recognized.

* chore(package): update react-ace to version 5.0.1 (Semantic-Org#1712)

https://greenkeeper.io/

* fix(Dropdown): add addition item key (Semantic-Org#1727)

* fix(factories): handle falsy `key` values (Semantic-Org#1729)

* fix(Dropdown): fix key handling

* fix(Dropdown): fix key handling

* fix(factories): handle falsy keys

* chore(package): update package-lock.json

* chore(package): update chai-enzyme to 0.7.1 (Semantic-Org#1731)

* feat(typings): expose FormComponent in typings (Semantic-Org#1680)

* Exposed form component in typings to support custom controls in packages.

* Update index.d.ts

* Update index.d.ts

* chore(package): update package-lock.json

* 0.68.5

* docs(changelog): update changelog [ci skip]

* fix(Input): add missing minLength prop (Semantic-Org#1734)

* docs(TableExampleSortable): pass in null when that column shouldn't be sorted (Semantic-Org#1737)

* feat(TextArea): add minHeight property, docs example (Semantic-Org#1679)

* add minHeight property, example to docs

* add rows prop/adjust minHeight prop usage

* mixed(TextArea): update docs, tests, props and typings

* fix(Textarea): move back minHeight prop to style

* fix(htmlInputProps): fix handle on falsy values (Semantic-Org#1746)

* fix(Search): Allow default action if there is no selected result (Semantic-Org#1742)

* docs(images): add missing images, update urls (Semantic-Org#1763)

* fix(Accordion): typings inverted to boolean (Semantic-Org#1758)

Tiny fix for typings, inverted type incorrectly set to string.

* feat(Icon): add ability use the loading prop without an icon (Semantic-Org#1768)

* feat(Button): add focus method (Semantic-Org#1764)

* fix(Dropdown): change active item on keyboard up/down (Semantic-Org#1735)

* fix(Dropdown): change active item on keyboard up/down

* fix(Dropdown): change active item on keyboard up/down

* refactor(Dropdown): simplify move constant

* refactor(Dropdown): remove hidden select (Semantic-Org#1730)

breaking(Dropdown): remove hidden select

* fix(Checkbox|Input): fix handling of aria-attributes (Semantic-Org#1752)

* fix(Checkbox|Input): fix handling of aria-attributes

* feat(htmlInputProps): update handling of aria
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants