Skip to content
This repository has been archived by the owner on May 2, 2023. It is now read-only.

refactor(knobs): Datepicker - Mandatory/optional knobs - FRONT-905 #390

Merged
merged 5 commits into from
Mar 27, 2020

Conversation

Joosthe
Copy link
Contributor

@Joosthe Joosthe commented Mar 25, 2020

PR description

Please drop a few lines about the PR: what it does, how to test it, etc.

QA Checklist

In order to ensure a safe and quick review, please check that your PR follow those guidelines:

  • I have put the vanilla component as devDependencies
  • I have put the specs package as devDependencies
  • I have added the components directly used in the twig file (with include or embed) as dependencies
  • My component is listed in @ecl-twig/ec-components's dependencies
  • My variables naming follow the guidelines (snake case for twig)
  • I have provided tests
  • I have provided documentation (for the "notes" tab)
  • If my local yarn.lock contains changes, I have committed it
  • I have given my PR the proper label (pr: review needed to indicate that I'm done and now waiting for a review ,pr: wip to indicate that I'm actively working on it ...)

@Joosthe Joosthe changed the title refactor(knobs): added knobs - FRONT-905 refactor(knobs): Datepicker - Mandatory/optional knobs - FRONT-905 Mar 25, 2020
@planctus
Copy link
Contributor

@Joosthe this ticket:
https://webgate.ec.europa.eu/CITnet/jira/browse/FRONT-905
was flagged and with a comment saying that there is no spec page in ECL:
https://ec.europa.eu/component-library/ec/components/forms/datepicker/usage/

i see it has not been added so i wonder what you based this work on.

@Joosthe
Copy link
Contributor Author

Joosthe commented Mar 25, 2020

Since the knobs were already there i set the knobs i based the cases on the variants. the optional elements are the elements that are visible if one of the cases is true

@Joosthe Joosthe added pr: review needed Use this label to show that your PR needs to be review tag: enhancement labels Mar 25, 2020
@planctus
Copy link
Contributor

Yep, @Joosthe , i thought i edited the comment replacing the "i see it has not been added so i wonder what you based this work on." with " i see that what you did makes sense, i will unflag the ticket" right after taking a look at the code, but i didn't publish it sorry, i removed the flag on the ticket instead, that i did it for real.. :)

@planctus planctus added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Mar 26, 2020
data.optional_text,
buttonLabels.optional
);
data.placeholder = text(
Copy link
Contributor

Choose a reason for hiding this comment

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

i think for now you can move this into the optional parameters, as we know there is no documentation page to base this on, but yeah, it looks more like an "option" than a requirement

Copy link
Contributor

Choose a reason for hiding this comment

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

You haven't changed this, but since there is no official docs about what is required and what is not for this component we can leave this as required.

@Joosthe Joosthe added pr: review needed Use this label to show that your PR needs to be review and removed pr: modification needed labels Mar 27, 2020
@planctus planctus added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Mar 27, 2020
@planctus planctus merged commit 2b86731 into develop Mar 27, 2020
@planctus planctus deleted the FRONT-905 branch March 27, 2020 09:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants