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

feat: EC component select - INNO-1340 #42

Merged
merged 9 commits into from
Mar 1, 2019
Merged

Conversation

emeryro
Copy link
Contributor

@emeryro emeryro commented Feb 28, 2019

PR description

Add component select (EC only)

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 ...)

@emeryro emeryro added the pr: wip Mark the PR as WIP label Feb 28, 2019
@emeryro emeryro self-assigned this Feb 28, 2019
@emeryro emeryro added pr: review needed Use this label to show that your PR needs to be review and removed pr: wip Mark the PR as WIP labels Feb 28, 2019
@emeryro emeryro removed their assignment Feb 28, 2019
Copy link
Contributor

@kalinchernev kalinchernev left a comment

Choose a reason for hiding this comment

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

In fact, it's rendering well, good job!
Very minor comments added.

class="ecl-form-label{{- invalid ? ' ecl-form-label--invalid' -}}{{- hide_label ? ' ecl-form-label--hidden'-}}"
for={{- _id -}}
>
{% block label _label %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing dash :)

>
{% block label _label %}
</label>
{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

The dash.


renderTwigFile(
template,
{
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not an issue for the size of this component. I'd suggest you either use the spec demo data object as a start and then use Object.assign to modify for specific scenarios, adding properties for invalid_* and helper_text for instance, things which are not given from the reference demo data.

@kalinchernev kalinchernev added pr: modification needed and removed pr: review needed Use this label to show that your PR needs to be review pr: under review labels Feb 28, 2019
@kalinchernev kalinchernev removed their assignment Feb 28, 2019
@emeryro emeryro added pr: wip Mark the PR as WIP and removed pr: modification needed labels Mar 1, 2019
@emeryro emeryro self-assigned this Mar 1, 2019
@emeryro emeryro added pr: review needed Use this label to show that your PR needs to be review and removed pr: wip Mark the PR as WIP labels Mar 1, 2019
@emeryro emeryro removed their assignment Mar 1, 2019
@emeryro emeryro added pr: wip Mark the PR as WIP and removed pr: review needed Use this label to show that your PR needs to be review labels Mar 1, 2019
@emeryro emeryro self-assigned this Mar 1, 2019
@emeryro emeryro added the pr: review needed Use this label to show that your PR needs to be review label Mar 1, 2019
@emeryro emeryro removed the pr: wip Mark the PR as WIP label Mar 1, 2019
@emeryro emeryro removed their assignment Mar 1, 2019
@kalinchernev kalinchernev merged commit 0edcd99 into master Mar 1, 2019
@yhuard yhuard self-assigned this Mar 1, 2019
@kalinchernev kalinchernev deleted the ec-component-select branch March 1, 2019 08:57
@yhuard yhuard added pr: under review and removed pr: review needed Use this label to show that your PR needs to be review labels Mar 1, 2019
{%- if _label is not empty -%}
<label
class="ecl-form-label{{- invalid ? ' ecl-form-label--invalid' -}}{{- hide_label ? ' ecl-form-label--hidden'-}}"
for={{- _id -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

what if _id contains a forbidden char?

The attribute value can remain unquoted if it doesn't contain ASCII whitespace or any of " ' ` = < or >. Otherwise, it has to be quoted using either single or double quotes.

https://html.spec.whatwg.org/multipage/introduction.html#intro-early-example

"@ecl/ec-component-form-help-block": "^2.1.1",
"@ecl/ec-component-form-label": "^2.1.1",
"@ecl/ec-component-form-select": "^2.1.1",
"@ecl/ec-resources-icons": "~2.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

mixing "~2.1.1" and "^2.1.1" and (in other components) "2.0.0" or "~2.1.0"

we have to harmonize all that. let's discuss it on slack 😉

@yhuard yhuard removed their assignment Mar 1, 2019
@yhuard yhuard removed pr: modification needed pr: review needed Use this label to show that your PR needs to be review pr: under review labels Mar 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants