Adds Input suffixes and prefixes #77

andy-polhill · 2020-11-23T21:47:21Z

Description

Initially raising this as a draft PR to get some visibility on it, it proved to be slightly larger than expected and any help getting it over the line would be most appreciated. It's probably worth reviewing one commit at a time.

Outstanding work

Check up on a11y best practice
Cross browser testing
Self review of the code

Requirements

Create the ability to add input prefixes and suffixes to input fields which may contain buttons. To enable this work a couple of smaller commits were made.

Addition of a small button variant
Addition of a small spinner variant
Addition of disabled property to button
Addition of an 'add-on' button variant.

As you can see from the designs the buttons are within the borders of the input field. This left me with two possible implementation options.

Absolutely position the input suffix/prefix inside the field. This would work except that the suffix could contain text and therefore be of a variable size, we would have to use JavaScript to detect the size the suffix before offsetting it
Add a wrapper around the input field, and move the focus events up to the wrapping element. We can then put the input fields, suffixes and prefixes inside that wrapper and use flexbox to control them.

I ended up going with option 2 as I think option 1 would lead to a flicker as the JavaScript calculated the suffix width.

From an a11y point of view I have linked the prefix and suffix to the described-by attribute of the input field. I don't treat the button any different from the text span, although I think a bit more testing on this would be nice. There is a similar discussion on GOV.uk at the moment

https://deploy-preview-77--legal-and-general-canopy.netlify.app/?path=/story/components-form-input--with-button-suffix

https://legalandgeneral.invisionapp.com/d/main#/console/19910472/416751159/preview

Checklist:

The commit messages follow the convention for this project
I have provided an adequate amount of test coverage
I have added the functionality to the test app
I have provided a story in storybook to document the changes
I have provided documentation in the notes section of the story
I have added any new public feature modules to public-api.ts
I have linked the new component to adobe DSM (if appropriate)

alexcanning · 2020-11-24T11:40:32Z

Should I be able to do this?

alexcanning · 2020-11-24T11:41:01Z

Focus on secondary buttons falls beneath primary button

andy-polhill · 2020-11-25T10:24:13Z

#77 (comment)
I fixed the focus issue.

andy-polhill · 2020-11-25T10:33:32Z

#77 (comment)

You can in theory put more suffixes in than you would ever need, there is some responsibility on the user to be sensible. I fixed the alignment issue in that screen shot though.

elenagarrone

Great work! I just had few small comments plus the followings for storybook:

the with button: the button doesn't have text so the screen reader only reads "button"
the with suffix: the example has the percentage but the label is still "Name" which makes no sense ;) this is just me being annoying :D
the with prefix: same as per above comment
the with multiple button suffixes: both the buttons don't have text so nothing is read to the screen readers apart from "button"

package.json

projects/canopy/src/lib/button/button.component.ts

projects/canopy/src/lib/button/button.interface.ts

projects/canopy/src/lib/button/button.notes.ts

elenagarrone · 2020-11-25T18:06:08Z

projects/canopy/src/lib/button/button.stories.ts

      [iconButton]="iconButton"
      [iconPosition]="iconPosition"
      [loading]="loading"
-      [fullWidth]="fullWidth"
+      [size]="size"


I'm wondering if maybe we should have a single size input that includes fullWidth instead of having them as two separate inputs. They both affect the size of the button so I think it makes sense 🤔

I guess the downside of doing this is that you may want a button to be both sm and fullWidth, which would be easy to do with separate inputs. If they were combined, we may need more options such as sm-full-width, md-full-width and so on, which could get long.

It could help reduce the number of options, links a bit to this issue.. https://github.com/canopy-collective/canopy/issues/251. I didn't actually change anything around that on this PR though, I just put it into alphabetical order. (which I realise doesn't make it easier for the reviewer 😀 )

elenagarrone · 2020-11-25T18:09:56Z

projects/canopy/src/lib/forms/input/input-field.component.ts

+    return this._prefixChildren;
+  }
+
+  @Input() id = `lg-input-${nextUniqueId++}`;


I think it would be better to save nextUniqueId++ into a separate const and then use it, otherwise if we end up reusing it, it will increase each time.

This was mostly reformatting and not changed in the PR. I'm open to changing it though but I'm not sure I quite follow. The idea is that it should increase each time, it was a convention I shamefully borrowed from material ui

Yes, it should increase each time a new component is instantiated.
I have had to fix multiple instances where we did the following mistake:

@Input() id = `lg-input-${nextUniqueId++}`; anotherVariable = `my-variable-${nextUniqueId++}`

which produces:

@Input() id = `lg-input-1`; anotherVariable = `my-variable-2`

while it should be:

@Input() id = `lg-input-1`; anotherVariable = `my-variable-1`

I know in this component this is not an issue atm but it did happen multiple times and I'm just concerned that someone might come along and do the same.

ah ok that makes sense, so in theory with the first example it still works as the IDs are unique, but it's harder to ascertain which elements join together as the numerical part of the id is different. (pushed a change)

elenagarrone · 2020-11-25T18:10:42Z

projects/canopy/src/lib/forms/input/input-field.component.ts

+  private hasHover = false;
+  private disabledStateChanges: Subscription;
+
+  ngAfterContentInit() {


I would like us to start using return types a bit more. Could you please add the return types for these functions?

Good idea, I've added a bunch to this file, I wonder if there is a lint warning we can add?

Yes I'm sure there is as we have it in our project. Might be one for when we create the global linting?

This additional variant should not be used on it's own, but it is useful to keep the same spacing as the button. For example suffix controls within input fields

owensgit · 2020-11-24T10:19:23Z

projects/canopy/src/lib/forms/input/input-field.component.scss


 .lg-input-field {
+  $block: &;


Not 100% sure the $block name works well, I wonder if $field would be better? Not super fussed either way on this though :)

I went for block as in the block inBEM, but in hindsight that may be misread, did you read it as block like display:block?

Yeah it took me a couple of takes to realise what was going on, maybe renaming it could make the code easier to scan. Then again, it's pretty clear once you know what's going on. So happy either way really :)

projects/canopy/src/lib/button/button.interface.ts

owensgit · 2020-11-26T10:02:39Z

projects/canopy/src/lib/button/button.stories.ts

      [iconButton]="iconButton"
      [iconPosition]="iconPosition"
      [loading]="loading"
-      [fullWidth]="fullWidth"
+      [size]="size"


I guess the downside of doing this is that you may want a button to be both sm and fullWidth, which would be easy to do with separate inputs. If they were combined, we may need more options such as sm-full-width, md-full-width and so on, which could get long.

andy-polhill · 2020-11-26T17:18:10Z

@elenagarrone on these ones...

the with button: the button doesn't have text so the screen reader only reads "button"
the with suffix: the example has the percentage but the label is still "Name" which makes no sense ;) this is just me being annoying :D
the with prefix: same as per above comment
the with multiple button suffixes: both the buttons don't have text so nothing is read to the screen readers apart from "button"

I've hopefully fixed the first one I was accidentally not rendering the text when it was an icon button. On the other ones I've tweaked the default text for those examples, people could still override them with silly things. I'm hoping by fixing the first one I've also fixed the last one, but there was text in the close button, so I'm slightly surprised that didn't read out, I might need to borrow your screen reader skills.

elenagarrone · 2020-11-27T09:57:39Z

@elenagarrone on these ones...

the with button: the button doesn't have text so the screen reader only reads "button"
the with suffix: the example has the percentage but the label is still "Name" which makes no sense ;) this is just me being annoying :D
the with prefix: same as per above comment
the with multiple button suffixes: both the buttons don't have text so nothing is read to the screen readers apart from "button"

I've hopefully fixed the first one I was accidentally not rendering the text when it was an icon button. On the other ones I've tweaked the default text for those examples, people could still override them with silly things. I'm hoping by fixing the first one I've also fixed the last one, but there was text in the close button, so I'm slightly surprised that didn't read out, I might need to borrow your screen reader skills.

The text in the first and last button is still not read and the reason is that you've set the lg-btn-text to have visibility: hidden. Most screen readers will not read elements with visibility: hidden or display: block. I would suggest you use the mixin lg-visually-hidden or the class .lg-visually-hidden which will allow screen readers to read the text.

andy-polhill · 2020-11-27T11:37:30Z

#77 (comment)
Good spot on this @elenagarrone , it was actually a bug on the original icon button implementation, I have tagged a separate commit onto this PR to fix it.

elenagarrone

Great work 👍

BREAKING CHANGE: - The input.scss styles have been wrapped into the main 'input-field.component.scss' file, if you were referencing this file directly any references may need updating - The input button types have changed to be more specific. If you were referencing the types directly you will need to update the following: `Variant` => `ButtonVariant` `Behaviour` => `ButtonBehaviour`

There are a couple of instances where we use Array.includes in the components, consumers will need to manually polyfill this. We should either mention this in the docs or modify the code.

Previously the button text was set to visibility:hidden; this was preventing it being read out on screen readers, updated to use the lg-visually-hidden mixin which is more screen reader friendly.

owensgit

Great addition Andy! 👍

github-actions · 2020-11-27T16:14:36Z

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

andy-polhill added the enhancement New feature or request label Nov 24, 2020

andy-polhill requested a review from a team November 24, 2020 09:29

andy-polhill force-pushed the input-with-button branch 3 times, most recently from 059b8eb to 30cd041 Compare November 25, 2020 10:18

andy-polhill force-pushed the input-with-button branch from c3c210b to 2cf8742 Compare November 25, 2020 10:32

andy-polhill marked this pull request as ready for review November 25, 2020 10:35

andy-polhill requested review from aaronBery, elenagarrone and owensgit as code owners November 25, 2020 10:35

elenagarrone requested changes Nov 25, 2020

View reviewed changes

andy-polhill added 6 commits November 26, 2020 09:58

feat(spinner): adds small spinner variant

db29d8c

feat(button): adds small variant

2dab1b9

fix(button): fixes disabled button styles

93f2fa3

feat(button): adds disabled input property

39721a6

refactor(button): adds Button prefix to Variant and Behaviour enums

3b085a2

feat(button): adds add-on only variant

1af8307

This additional variant should not be used on it's own, but it is useful to keep the same spacing as the button. For example suffix controls within input fields

andy-polhill force-pushed the input-with-button branch from 2cf8742 to 30ef635 Compare November 26, 2020 09:59

owensgit reviewed Nov 26, 2020

View reviewed changes

owensgit requested review from owensgit and elenagarrone November 26, 2020 15:24

owensgit previously approved these changes Nov 26, 2020

View reviewed changes

andy-polhill dismissed owensgit’s stale review via a8e0a38 November 26, 2020 17:15

elenagarrone previously approved these changes Nov 27, 2020

View reviewed changes

andy-polhill added 5 commits November 27, 2020 11:48

test(test-app): adds css ponyfill support for IE11

4ede87f

test(test-app): adds polyfills for IE11

d43955d

There are a couple of instances where we use Array.includes in the components, consumers will need to manually polyfill this. We should either mention this in the docs or modify the code.

fix(input-field): adds input field prefix and suffixes to described-by

4a5295f

fix(button): fixes icon only text for screen readers

77f88c6

Previously the button text was set to visibility:hidden; this was preventing it being read out on screen readers, updated to use the lg-visually-hidden mixin which is more screen reader friendly.

andy-polhill dismissed elenagarrone’s stale review via 77f88c6 November 27, 2020 11:48

andy-polhill force-pushed the input-with-button branch from 3538e30 to 77f88c6 Compare November 27, 2020 11:48

elenagarrone approved these changes Nov 27, 2020

View reviewed changes

owensgit self-requested a review November 27, 2020 16:07

owensgit approved these changes Nov 27, 2020

View reviewed changes

owensgit merged commit 8c24443 into Legal-and-General:master Nov 27, 2020

andy-polhill deleted the input-with-button branch November 27, 2020 16:14

github-actions bot added the released label Nov 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Input suffixes and prefixes #77

Adds Input suffixes and prefixes #77

andy-polhill commented Nov 23, 2020 •

edited

Loading

alexcanning commented Nov 24, 2020

alexcanning commented Nov 24, 2020

andy-polhill commented Nov 25, 2020

andy-polhill commented Nov 25, 2020

elenagarrone left a comment

elenagarrone Nov 25, 2020

owensgit Nov 26, 2020

andy-polhill Nov 26, 2020 •

edited

Loading

elenagarrone Nov 25, 2020

andy-polhill Nov 26, 2020

elenagarrone Nov 27, 2020

andy-polhill Nov 27, 2020

elenagarrone Nov 25, 2020

andy-polhill Nov 26, 2020

elenagarrone Nov 27, 2020

owensgit Nov 24, 2020

andy-polhill Nov 26, 2020

owensgit Nov 26, 2020

owensgit Nov 26, 2020

andy-polhill commented Nov 26, 2020

elenagarrone commented Nov 27, 2020 •

edited

Loading

andy-polhill commented Nov 27, 2020

elenagarrone left a comment

owensgit left a comment

github-actions bot commented Nov 27, 2020

Adds Input suffixes and prefixes #77

Adds Input suffixes and prefixes #77

Conversation

andy-polhill commented Nov 23, 2020 • edited Loading

Description

Requirements

Checklist:

alexcanning commented Nov 24, 2020

alexcanning commented Nov 24, 2020

andy-polhill commented Nov 25, 2020

andy-polhill commented Nov 25, 2020

elenagarrone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-polhill Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-polhill commented Nov 26, 2020

elenagarrone commented Nov 27, 2020 • edited Loading

andy-polhill commented Nov 27, 2020

elenagarrone left a comment

Choose a reason for hiding this comment

owensgit left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 27, 2020

andy-polhill commented Nov 23, 2020 •

edited

Loading

andy-polhill Nov 26, 2020 •

edited

Loading

elenagarrone commented Nov 27, 2020 •

edited

Loading