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

Add helper text API and slot #410

Merged
merged 21 commits into from Oct 9, 2019
Merged

Add helper text API and slot #410

merged 21 commits into from Oct 9, 2019

Conversation

niiyeboah
Copy link

@niiyeboah niiyeboah commented Sep 11, 2019

  • Added helper-text part below text field
  • Added observer to add has-helper attribute when helperText property is set
  • Added slotchange event handler to add has-helper attribute when content placed in helper slot
  • Added helper text to aria-describedby
  • Added observer for error message to add has-error-message attribute (used to hide helper in material theme when error message is set)

src/vaadin-text-field-mixin.html Outdated Show resolved Hide resolved
@niiyeboah niiyeboah changed the title Feature/hint text Add helper text API and slot Sep 21, 2019
src/vaadin-text-field-mixin.html Outdated Show resolved Hide resolved
src/vaadin-text-field-mixin.html Show resolved Hide resolved
src/vaadin-text-field-mixin.html Show resolved Hide resolved
src/vaadin-text-field-mixin.html Show resolved Hide resolved
Copy link
Contributor

@yuriy-fix yuriy-fix left a comment

Choose a reason for hiding this comment

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

Good job! 👍

Some nits:
Should we consider updating the indents when label is used together with helper text?
Screenshot 2019-10-03 at 12 52 56

Due to the order usage in styles, error-message is also changing the position with theme helper-above-field. Is it intended?
Screenshot 2019-10-03 at 13 04 24

Should the helper text be mentioned on the screen readers (not mentioned currently on Voice over).
Screenshot 2019-10-03 at 12 59 55

theme/material/vaadin-text-field-styles.html Outdated Show resolved Hide resolved

it('text-field with slotted helper updates has-helper attribute', function(done) {
const textFieldSlottedHelper = fixture('default-with-slotted-helper');
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can avoid timeouts by using similar approach as in text-field with dispatching slot-change:

function dispatchSlotChange() {

Copy link
Author

Choose a reason for hiding this comment

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

Done, except it didn't work with this test:

it('removing slotted helper removes has-helper attribute', function(done) {
const textFieldSlottedHelper = fixture('default-with-slotted-helper');
const helper = textFieldSlottedHelper.querySelector('[slot="helper"]');
textFieldSlottedHelper.removeChild(helper);
// dispatchHelperSlotChange does not work with this test
setTimeout(() => {
expect(textFieldSlottedHelper.hasAttribute('has-helper')).to.be.false;
done();
}, 1);
});

Copy link
Member

Choose a reason for hiding this comment

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

Hmm...for me the same approach works in that test also

@niiyeboah
Copy link
Author

Thanks Yuriy :)

I fixed the error message position and also the label padding when the helper text is above (I agree it looks better):

Screenshot 2019-10-04 at 13 17 38

When I test the voice over, the helper text is read after the label, and then if there is an error message it will be read after that. There is however some weird behaviour which happens on my system. When the helper is a slot it seems to cause the page to reload sometimes if you have voice over enabled and focus on the element.

Screenshot 2019-10-04 at 13 12 16

Screenshot 2019-10-04 at 13 12 28

@tomivirkki tomivirkki changed the base branch from master to 2.6 October 8, 2019 13:31
src/vaadin-text-field-mixin.html Outdated Show resolved Hide resolved
src/vaadin-text-field-mixin.html Outdated Show resolved Hide resolved
@tomivirkki tomivirkki merged commit c48aa48 into 2.6 Oct 9, 2019
@tomivirkki tomivirkki deleted the feature/hint-text branch October 9, 2019 11:12
tomivirkki pushed a commit that referenced this pull request Oct 9, 2019
@tomivirkki tomivirkki added this to Candidates for Vaadin 14.2 in No longer in use, go to https://vaadin.com/roadmap Oct 17, 2019
@juhopiirainen juhopiirainen moved this from Candidates for Vaadin 14.2 to Work in Progress in No longer in use, go to https://vaadin.com/roadmap Oct 21, 2019
tomivirkki added a commit that referenced this pull request Apr 6, 2020
This reverts commit d719168.

# Conflicts:
#	src/vaadin-text-field-mixin.html
#	theme/lumo/vaadin-text-field-styles.html
tomivirkki added a commit that referenced this pull request Apr 7, 2020
This reverts commit d719168.

# Conflicts:
#	src/vaadin-text-field-mixin.html
#	theme/lumo/vaadin-text-field-styles.html
DiegoCardoso pushed a commit that referenced this pull request May 25, 2020
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

3 participants