Start DateInput by Ladsgroup · Pull Request #398 · wmde/wikit

Ladsgroup · 2021-03-18T12:28:13Z

github-actions · 2021-03-18T12:28:41Z

https://9cf6fece17188cab031723f2cf84edfeb5444e91--wmde-wikit.netlify.app

micgro42

looks good so far 👍

micgro42 · 2021-03-18T13:51:32Z

+					<b>{{ parsedValue }}</b>
+				</p>
+				<BouncingDots v-if="!parsedValue && value" type="small" />
+				<div class="wikit-DateInput--CalendarNotice" v-if="calendarNotice && parsedValue">


I think this class should be wikit-DateInput__CalendarNotice - it is an element, not a modifier, right?

Yes, I'm terrible at BEM. Fixed now.

micgro42 · 2021-03-18T17:15:39Z

+			:label="label"
+			:value="value"
+			:error="error"
+			:feedback-type="feedbackType"


InputWithExtender doesn't have a prop "feedback" type. The error prop is good enough

oh nice. Thanks. done now.

micgro42

Solid job! just one little detail to fix and it can have my approval 👍

sai-san · 2021-03-19T13:13:26Z

This is really great! 💯

There's a Reference error that prevents the Basic story from being displayed in Chromatic. Adding some comments here after checking the components in the All story:

For some reason, the IntroText style is not being applied to the default prompt in Chrome & Safari on MacOs (the style looks good in Firefox/Linux, though):

The variants in the All page should have some more vertical space between them (see the All story of the ExtendedNumberInput for reference). I believe that the spacing is achieved by containing the variants in paragraphs (which is probably also not ideal).
In the design specification, it was indicated that text elements inside the Extender menu should be contained in 24px high "bounding boxes" (aka divs) verticall-spaced by 4px. Checking the current implementation, I'm not sure if that's applied or even if it's necessary, but definitely the vertical spacing between elements should be increased, to probably 8px (dimension.spacing.small.value). I wasn't sure where to suggest the change

sai-san · 2021-03-30T08:46:52Z

Charlie made a good point regarding the default active state of the DateInput component that made me think the interaction flow could be improved in the following way:

Current: Once the user focuses on the DateInput, the extender menu opens to display the prompt text.
Desired: Once the user focuses on the DateInput, instead of just the prompt, users also see the resultsIntroText. In the following way:

This removes a step from the flow and concentrates all interaction in a single menu, that now displays the different states (pre-input, during input, and result).

Since the component is still in the last stages of development, I thought we still are on time to make this improvement 🙏

Ladsgroup · 2021-03-30T12:27:18Z

This is really great! 100

There's a Reference error that prevents the Basic story from being displayed in Chromatic. Adding some comments here after checking the components in the All story:
* For some reason, the IntroText style is not being applied to the default prompt in Chrome & Safari on MacOs (the style looks good in Firefox/Linux, though):
* The variants in the All page should have some more vertical space between them (see the All story of the ExtendedNumberInput for reference). I believe that the spacing is achieved by containing the variants in paragraphs (which is probably also not ideal).

Added <br> in between.

* In the design specification, it was indicated that text elements inside the Extender menu should be contained in 24px high "bounding boxes" (aka divs) verticall-spaced by 4px. Checking the current implementation, I'm not sure if that's applied or even if it's necessary, but definitely the vertical spacing between elements should be increased, to probably 8px (dimension.spacing.small.value). I wasn't sure where to suggest the change

Isn't it $wikit-InputExtender-padding-vertical?

sai-san · 2021-03-30T13:12:28Z

Thanks for all the changes and fixes, @Ladsgroup!

For some reason, the IntroText style is not being applied to the default prompt in Chrome & Safari on MacOs (the style looks good in Firefox/Linux, though):

This error still persists: the text-styles applied to the prompt are ignored in MacOS (checked Safari, Firefox and Chrome)

Isn't it $wikit-InputExtender-padding-vertical?

Yes, that token is defining the internal padding of the menu, but the elements inside are unaffected. Maybe we should define their spacing separately, like in the options menu? I can update the tokens file (#400)

sai-san · 2021-03-30T15:17:23Z

+				<slot v-if="!value" name="prompt" />
+				<BouncingDots v-if="!parsedValue && value" type="small" />
+				<div class="wikit-DateInput__CalendarNotice" v-if="calendarNotice && parsedValue">
+					<Icon type="info" size="small" />


can we apply color="subtle" to the icon or make it inherit the color of the text it accompanies? Any of the options are fine

We don't have "subtle" color type. The ones are 'base', 'warning', 'error', 'notice', 'success', 'inherit'. I change it to inherit.

thanks! inherit makes sense. I'm opening a small PR to add those missing colors to the icon component (I was convinced they were there, but maybe mixed up colors and sizes), but no further change is needed here.

sai-san · 2021-03-30T16:15:25Z

yay! looks so good! I would approve right away if it weren't (again) for that missing style:

Is there anything we can try there? Should we create a class for that text element too? The style literally matches that of the input placeholder.

micgro42 · 2021-04-01T09:50:32Z

+				<p class="wikit-DateInput__ParsedValue" v-if="parsedValue">
+					<b>{{ parsedValue }}</b>
+				</p>
+				<slot v-if="!value" name="prompt" />


Not blocking this PR: But since we now show the intro text always, do we want the prompt to actually still be a slot? That is where the application can inject whatever markup it wants. Or would we not prefer for this now to be a text-only prop? @SaiSan-WMDE

Though, this is not blocking the merge and can still be adjusted later on.

@Ladsgroup and I just discussed this and originally agreed to keep the slot. But you're right, this is some static text that will always be displayed, so using a text-only prop like in the other cases makes more sense and is more consistent?

You tell me 🙃
This really depends on what you envision might be inserted there by the application.
If it is only ever going to be plain text that is always supposed to be styled the same way, then a prop makes more sense.
On the other hand, if you want to keep the possibility open that the application might want to provide some copy that includes a "learn more" link, then we should keep the slot.

If you two discussed this and decided that to keep the slot, then I'm confident that this is the right decision and am very happy to approve this as soon as we have the note in the CHANGELOG.md :)

I believe this should be a text prop for now, since I cannot foresee any use cases that justify the use of a slot at the moment. A slot could be introduced later if that changed.

Last change and we should be done with this component!

Implemented as requested.

micgro42 · 2021-04-01T09:57:48Z

This still needs a line in the CHANGELOG.md, I think :)

Bug: T273002

Co-authored-by: Sai_San <sarai.sanchez@wikimedia.de>

As requested by UX.

sai-san

Just gorgeous. Thanks, Michael and Amir! 💯

Ladsgroup force-pushed the date_component branch from 6ab3b52 to a1e9865 Compare March 18, 2021 12:39

micgro42 reviewed Mar 18, 2021

View reviewed changes

Ladsgroup force-pushed the date_component branch from a1e9865 to 79c88f3 Compare March 18, 2021 15:52

Ladsgroup marked this pull request as ready for review March 18, 2021 15:55

Ladsgroup requested review from bereket-WMDE, guergana and sai-san March 18, 2021 16:00

micgro42 reviewed Mar 18, 2021

View reviewed changes

Ladsgroup force-pushed the date_component branch from 79c88f3 to 9b52687 Compare March 19, 2021 03:55

Ladsgroup force-pushed the date_component branch from 9b52687 to 6b36d1d Compare March 30, 2021 12:29

sai-san reviewed Mar 30, 2021

View reviewed changes

Comment thread vue-components/src/components/DateInput.vue Outdated

sai-san reviewed Mar 30, 2021

View reviewed changes

Comment thread vue-components/src/components/DateInput.vue Outdated

sai-san reviewed Mar 30, 2021

View reviewed changes

Comment thread vue-components/src/components/DateInput.vue

sai-san reviewed Mar 30, 2021

View reviewed changes

Comment thread vue-components/src/components/DateInput.vue

Ladsgroup force-pushed the date_component branch from ab6c389 to 14cc24e Compare March 30, 2021 15:51

wmde deleted a comment from Ladsgroup Mar 30, 2021

Ladsgroup force-pushed the date_component branch from 14cc24e to e2cba29 Compare March 30, 2021 16:02

micgro42 reviewed Apr 1, 2021

View reviewed changes

micgro42 force-pushed the date_component branch from 2596d3a to 5fdbfce Compare April 1, 2021 14:56

Ladsgroup and others added 2 commits April 1, 2021 16:56

Start DateInput

ef8d817

Bug: T273002

Update vue-components/src/components/DateInput.vue

e9dee21

Co-authored-by: Sai_San <sarai.sanchez@wikimedia.de>

sai-san and others added 4 commits April 1, 2021 16:56

Update vue-components/src/components/DateInput.vue

afe678c

Add more fixes

6204d61

Update CHANGELOG.md

757dcbb

Use prop instead of slot for prompt

9c59810

As requested by UX.

micgro42 force-pushed the date_component branch from 5fdbfce to 9c59810 Compare April 1, 2021 14:57

sai-san self-requested a review April 1, 2021 15:13

sai-san approved these changes Apr 1, 2021

View reviewed changes

micgro42 approved these changes Apr 1, 2021

View reviewed changes

Ladsgroup merged commit 42bd210 into master Apr 1, 2021

Ladsgroup deleted the date_component branch April 1, 2021 19:03

Conversation

Ladsgroup commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Mar 18, 2021

Uh oh!

micgro42 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micgro42 left a comment

Choose a reason for hiding this comment

Uh oh!

sai-san commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sai-san commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ladsgroup commented Mar 30, 2021

Uh oh!

sai-san commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sai-san Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sai-san commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sai-san Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micgro42 commented Apr 1, 2021

Uh oh!

sai-san left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Ladsgroup commented Mar 18, 2021 •

edited

Loading

sai-san commented Mar 19, 2021 •

edited

Loading

sai-san commented Mar 30, 2021 •

edited

Loading

sai-san commented Mar 30, 2021 •

edited

Loading

sai-san Mar 31, 2021 •

edited

Loading

sai-san commented Mar 30, 2021 •

edited

Loading

sai-san Apr 1, 2021 •

edited

Loading

sai-san left a comment •

edited

Loading