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

feat: add QuantityInput component #378

Merged
merged 16 commits into from Mar 10, 2021
Merged

feat: add QuantityInput component #378

merged 16 commits into from Mar 10, 2021

Conversation

micgro42
Copy link
Collaborator

wip

#377 needs to be merged first because this PR here needs it validation functionality.

@github-actions
Copy link

@micgro42 micgro42 force-pushed the QuantityInput branch 3 times, most recently from 67d30c7 to ce89206 Compare February 22, 2021 14:44
@micgro42 micgro42 force-pushed the QuantityInput branch 5 times, most recently from 2b3edfe to dc786ec Compare March 3, 2021 18:02
@micgro42 micgro42 marked this pull request as ready for review March 3, 2021 18:06
@micgro42 micgro42 requested review from a team and sai-san March 3, 2021 18:06
@sai-san
Copy link
Collaborator

sai-san commented Mar 4, 2021

This looks great. I give it 1000 cabbage 🥬

I made some small changes to the style of the entered value/quantity indicators (as always), and replaced the error copies just to see if the ones indicated in the designs are clear enough. While doing that, I noticed the nit-pickiest thing: that the error message of the second QuantityInput (when the unit field is being validated) is not properly aligned like the rest. I wasn't sure what's causing it or how to fix it:

Screenshot 2021-03-04 at 10 51 40

Regarding the transparent end border applied to the first field: this make the component look closer to the designs, but the solution is a bit hacky. In case there's not a better way to achieve creating that shared 1px border, it might be better to make the border visible instead.

.wikit-Lookup__label {
position: absolute;
width: 1px;
height: 1px;
Copy link
Contributor

Choose a reason for hiding this comment

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

there are no tokens here ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There could be? But I don't get to understand what are they styling 🤔

Copy link
Collaborator Author

@micgro42 micgro42 Mar 9, 2021

Choose a reason for hiding this comment

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

Ah, this was hiding the Label from the Lookup component so that it is still readable to screen readers. But we're no longer using the Lookup, but the LookupInput, so this is now unused code and can be removed.

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

great

const enteredNumberText = '123';

wrapper.findComponent( Input ).vm.$emit( 'input', enteredNumberText );
await wrapper.vm.$nextTick();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe also here ?
see comment here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, done

@bereket-WMDE
Copy link
Contributor

P.S.
Its looks great, btw :)

@micgro42
Copy link
Collaborator Author

micgro42 commented Mar 9, 2021

While doing that, I noticed the nit-pickiest thing: that the error message of the second QuantityInput (when the unit field is being validated) is not properly aligned like the rest. I wasn't sure what's causing it or how to fix it:

Screenshot 2021-03-04 at 10 51 40

Mh, that looks perfectly fine to me:
image

What browser are you using and does this also happen on other validation messages?

@micgro42
Copy link
Collaborator Author

micgro42 commented Mar 9, 2021

Regarding the transparent end border applied to the first field: this make the component look closer to the designs, but the solution is a bit hacky. In case there's not a better way to achieve creating that shared 1px border, it might be better to make the border visible instead.

I have to confess, I'm only aware of different bad solutions.:

  • The transparent border allows us to actually show 4 sides of red border in case of an error. But that means in the case of an error there is also a double border there.
  • What we did for the ToggleButton group is to just drop the right border for all but the rightmost button. Luckily that is almost imperceptible unless one knows exactly where one has to look when.
  • double border always is always consistent but also always a double border.

bereket-WMDE
bereket-WMDE previously approved these changes Mar 9, 2021
@sai-san
Copy link
Collaborator

sai-san commented Mar 9, 2021

Mh, that looks perfectly fine to me:
image

What browser are you using and does this also happen on other validation messages?

Chrome on MacOS. This issue is tinny, though. Finding whatever might be causing it is the interesting part, but it shouldn't be a blocker from a UI perspective.

Regarding the transparent border, I understand how this was the only approach possible to translate the design..but looking at the result, I'd say that we should use the double border for now (although it's not ideal) to avoid both the inconsistencies between states and the hacky-looking corners:

Screenshot 2021-03-04 at 10 35 44

@micgro42 micgro42 requested a review from a team March 10, 2021 16:57
Copy link
Collaborator

@sai-san sai-san left a comment

Choose a reason for hiding this comment

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

Looking osom 🙌

@micgro42 micgro42 merged commit 65074e8 into master Mar 10, 2021
@micgro42 micgro42 deleted the QuantityInput branch March 10, 2021 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants