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

fix: fixes a few misc. problems with AutoComplete and updates the docs #214

Merged
merged 2 commits into from
Aug 18, 2020

Conversation

zero0Halo
Copy link
Contributor

@zero0Halo zero0Halo commented Aug 14, 2020

fix(AutoComplete): isFocused now receives its initial value from autoFocus prop

fix(AutoComplete): custom result story now spreads value to attach events

fix(AutoComplete): added size knob for StateBasedAutoCompleteStory

fix(Input): line heights given to inputs to fix cutoff of text

docs(autocomplete): better documentation on custom result templates

Before submitting a pull request, please make sure the following is done:

I have done all of the following:

  • Added a top level class to all my components '.anchor-[COMPONENT NAME]'.
  • Used conventional commits for all work.
  • Tested my solution on Mobile & Tablet.
  • Wrote unit tests for states and all behavior (npm test) and passed coverage thresholds.
  • Updated snapshots for all permutations (npm test -- -u).
  • Accounted for hover, focus, blur, visited, & error states because they are not edge cases.
  • Created TODOs for known edge cases.
  • Documented all of my changes (inline & doc site).
  • Made sure that all accessibility errors are resolved.
  • Added stories with knobs for all possible configurations.
  • De-linted and ran prettier (npm run pretty) on my code.
  • Added name to OWNERS file for all new components
  • If adding a new component, add its export to the rollup config
  • package.json version is bumped (if necessary)

Outline your feature or bug-fix below

@zero0Halo zero0Halo force-pushed the fix/autocomplete-fixes branch 2 times, most recently from bd3d137 to 610da4b Compare August 17, 2020 16:51
label: string;
onMouseOver: (event?: React.MouseEvent) => void;
onSelect: (event?: React.FocusEvent) => void;
[key: string]: any; // Value also contains any new fields passed via the dataSource prop
Copy link
Contributor

Choose a reason for hiding this comment

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

Replacing new fields with something like additional custom fields makes more sense to me.

value: {
active,
id,
isLink, // Here's the isLink key from the dataSource. All custom keys will be added to 'value'.
Copy link
Contributor

@mishahoo-zz mishahoo-zz Aug 17, 2020

Choose a reason for hiding this comment

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

And I think this can simply be flagged as a custom field // custom field from datasource

<ListItem active={index === currentIndex}>{label}</ListItem>
<ListItem active={active} key={key} onSelect={onSelect} onMouseOver={onMouseOver}>
<Typography weight={800} mr="2">{label}</Typography>
(active: {active.toString()}, currentIndex: {currentIndex}, id: {id}, key: {key} index: {index})
Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think it's interesting to see the extra info but not really helpful. And the font looks a little too bold.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree the info isn't super useful, but I wanted to convey (as clearly as I could) that these are all of the properties available. I can definitely tone down the bold, it's a bit much.

{ label: 'I\'m a Link!', id: 'list-item-2', isLink: true, },
{ label: 'Another List Item', id: 'list-item-3', },
{ label: 'And Another', id: 'list-item-4', },
{ label: '...You Get The Idea', id: 'list-item-5', },
Copy link
Contributor

@mishahoo-zz mishahoo-zz Aug 17, 2020

Choose a reason for hiding this comment

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

The labels are a little confusing to me. At first it seemed to me like you were saying that only the first results was a Custom Results Template and the others were not. I think keeping these unbolded and listed as Item , Item 2, etc... like it was before seems better to me. 🤷

…Focus prop

fix(AutoComplete): custom result story now spreads value to attach events

fix(AutoComplete): added size knob for StateBasedAutoCompleteStory

fix(Input): line heights given to inputs to fix cutoff of text

docs(autocomplete): better documentation on custom result templates

fix(build): explicitly added csstypes module

docs(autocomplete): changes made based on PR comments
@zero0Halo zero0Halo merged commit 3779c01 into master Aug 18, 2020
@zero0Halo zero0Halo deleted the fix/autocomplete-fixes branch August 18, 2020 21:27
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

2 participants