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(text-input): provide ecl-text-input class - INNO-520 #150

Merged
merged 9 commits into from
May 30, 2017

Conversation

yhuard
Copy link
Contributor

@yhuard yhuard commented May 30, 2017

@kalinchernev kalinchernev self-assigned this May 30, 2017

Markup: html/form-group.html
Copy link
Contributor

Choose a reason for hiding this comment

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

The ecl-forms-form-groups.twig still has old classes.

@@ -1,5 +1,5 @@
module.exports = {
title: 'Text inputs',
label: 'Text inputs',
status: 'planned',
status: 'ready',
Copy link
Contributor

Choose a reason for hiding this comment

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

The help-block part is still in progress though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll rework this

<label for="example-input-id-1">Text Input - default with placeholder</label>
<p class="help-block">This is some help text.</p>
<input type="text" class="form-control" id="example-input-id-1" placeholder="Some placeholder text."/>
<input type="text" class="ecl-text-input" id="example-input-id-1" placeholder="Some 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.

If possible, please order the ids and names.
Also, it seems that some examples are missing: https://ec-europa.github.io/ec-europa-theme/section-form.html#kssref-form-text-input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No missing examples actually: they made an example for the :focus state by adding a class for that... Do we want to follow this road?

@@ -1,8 +1,20 @@
{
"private": true,
"name": "@ec-europa/ecl-forms-text-inputs",
Copy link
Contributor

Choose a reason for hiding this comment

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

Either please add "@ec-europa/ecl-forms-help-blocks" as a dependency or maybe just remove it for the time being.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add it as a peerDependency, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be better now

@yhuard
Copy link
Contributor Author

yhuard commented May 30, 2017

Ready for a last review :)

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.

I think we can merge this one.

@yhuard yhuard merged commit 0210571 into master May 30, 2017
@yhuard yhuard deleted the feat/input-texts-INNO-520 branch May 30, 2017 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants