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

[ISSUE-677] Helpertext on combobox #723

Merged
merged 8 commits into from
Jun 10, 2019
Merged

[ISSUE-677] Helpertext on combobox #723

merged 8 commits into from
Jun 10, 2019

Conversation

samends
Copy link

@samends samends commented May 4, 2019

Type: feature

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:

  • Adds helpertext to combobox widget
  • Adds onValidate property to the combobox widget
  • Updates valid property to use most recent type interface {valid: boolean, message: string} instead of a boolean.

Contributes to #677

@samends samends added the draft label May 4, 2019
@samends samends self-assigned this May 4, 2019
@codecov
Copy link

codecov bot commented May 6, 2019

Codecov Report

Merging #723 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #723      +/-   ##
==========================================
- Coverage   98.41%   98.37%   -0.04%     
==========================================
  Files          44       48       +4     
  Lines        3273     3320      +47     
  Branches      970      980      +10     
==========================================
+ Hits         3221     3266      +45     
- Misses         52       54       +2
Impacted Files Coverage Δ
src/time-picker/index.ts 100% <ø> (ø) ⬆️
src/text-input/index.ts 98.64% <ø> (ø) ⬆️
src/combobox/index.ts 98.19% <100%> (-1.35%) ⬇️
src/slide-pane/index.ts 95.8% <0%> (-0.2%) ⬇️
src/grid/index.ts 85.71% <0%> (-0.19%) ⬇️
src/grid/processes.ts 93.75% <0%> (-0.08%) ⬇️
src/grid/Header.ts 98.5% <0%> (-0.07%) ⬇️
src/grid/Body.ts 97.27% <0%> (-0.05%) ⬇️
src/dialog/index.ts 100% <0%> (ø) ⬆️
... and 12 more

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 0ef5afa...1dfc014. Read the comment docs.

@samends samends removed the draft label May 6, 2019
@samends
Copy link
Author

samends commented May 6, 2019

Does this functional tests? I wasn't sure

valid: { valid: this._valid, message: 'Please enter value of state' },
helperText: 'helper text',
onValidate: (valid: boolean | undefined, message: string) => {
console.log('onValidate called', valid, message);
Copy link
Member

Choose a reason for hiding this comment

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

this callback should be used to set valid back on the widget

Copy link
Author

@samends samends May 20, 2019

Choose a reason for hiding this comment

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

What does the onValidate function do? I've been a bit confused there

Copy link
Author

Choose a reason for hiding this comment

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

And if the onValidate function is something that will update the valid boolean based on the most up to date version of the dom shouldn't that be abstracted away so the user doesn't have to think about that?

Copy link
Member

Choose a reason for hiding this comment

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

it's a controlled component, the onValidate is a callback to inform the parent that the valid status has changed. It's upto the parent to set valid back on the widget so that it can display the new status.

@tomdye
Copy link
Member

tomdye commented May 30, 2019

Looks good @sammenza , but I think you still need to remove the labelAfter property.
It doesn't make sense / work with helperText.

@samends
Copy link
Author

samends commented Jun 3, 2019

Alright cool, labelAfter has been removed

@samends samends merged commit a571a24 into dojo:master Jun 10, 2019
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.

2 participants