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): add experimental text-input #1074

Merged
merged 26 commits into from
Sep 21, 2018
Merged

feat(text-input): add experimental text-input #1074

merged 26 commits into from
Sep 21, 2018

Conversation

alisonjoseph
Copy link
Member

@alisonjoseph alisonjoseph commented Sep 9, 2018

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @alisonjoseph!

@tw15egan
Copy link
Collaborator

I'm getting a different hover action in my local environment... I could be checking it out wrong, all I need to do is flip the switch at the top, right?

dev

@alisonjoseph
Copy link
Member Author

Oops, the staging link is wrong. sorry! @tw15egan

This one definitely needs some design review/input.

@tw15egan
Copy link
Collaborator

Okay, going to wait on design review before doing code review, as things may change

@tw15egan
Copy link
Collaborator

@alisonjoseph can you add a preview link when you get a chance, so we can get design review?

@alisonjoseph
Copy link
Member Author

alisonjoseph commented Sep 17, 2018

I'm trying... my computer hates me 😢
screen shot 2018-09-17 at 2 34 26 pm

@alisonjoseph
Copy link
Member Author

alisonjoseph commented Sep 17, 2018

Screen shots for now, until I can figure out a staging link. Full disclosure that I just copied styles from duo-components for this and didn't check against any designs (because I don't have any)

screen shot 2018-09-17 at 2 50 02 pm

Hover State
screen shot 2018-09-17 at 2 50 06 pm

Focus State
screen shot 2018-09-17 at 2 50 12 pm

@alisonjoseph alisonjoseph requested a review from a team September 17, 2018 21:53
height: rem(40px);
padding: 0 $spacing-xl 0 $spacing-md;
color: $text-02;
border: none;
Copy link
Member

Choose a reason for hiding this comment

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

There should be a 1px border on all sides with $ui-04 as the border color
image

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

}

.#{$prefix}--text-input[data-invalid],
.#{$prefix}--text-input:invalid {
Copy link
Member

@aagonzales aagonzales Sep 18, 2018

Choose a reason for hiding this comment

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

What invalid state should look like according to duo.
image

  • no *
  • font-weight: regular/400
  • font-color: support-01
  • add error icon to the right inside the field
  • invalid message should only be 4px from the bottom of the field


.#{$prefix}--text-input[data-invalid],
.#{$prefix}--text-input:invalid {
outline: 1px solid $support-01;
Copy link
Member

Choose a reason for hiding this comment

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

outline should be 2px thick

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@alisonjoseph
Copy link
Member Author

@aagonzales Is the label text the correct size?

outline-offset: -2px;
box-shadow: none;
background: $field-01
url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32 32" width="16" height="16" class="ibm-icons ibm-icons--error--filled"><path fill="#da1e28" d="M6.95 26.66A14 14 0 0 0 26.67 6.93zM16 2A14 14 0 0 0 5.35 25.07L25.08 5.34A13.93 13.93 0 0 0 16 2z"/></svg>');
Copy link
Member Author

Choose a reason for hiding this comment

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

@IBM/carbon-developers thoughts on this for the error icon to avoid html changes for now?

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Nice progress @alisonjoseph!

package.json Outdated
@@ -167,6 +167,7 @@
"scripts": {
"build": "gulp build",
"build-dev": "gulp build:dev",
"rollup": "gulp build:dev --rollup -e",
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding in @alisonjoseph! Would it make sense to name it build-dev-rollup to make it align with build-dev? BTW -e is for building experimental version of style - Probably we'd create build-dev-rollup and build-dev-rollup-experimental, like:

"build-dev-rollup": "gulp build:dev --rollup",
"build-dev-rollup-experimental": "gulp build:dev --rollup -e",

What do you think? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, will update the name. Thanks for the suggestion.

.#{$prefix}--text-input:disabled {
cursor: not-allowed;
// THIS SHOULD BE MOVED TO AN INLINE SVG IN NEXT MAJOR RELEASE
background-image: url("data:image/svg+xml;charset=UTF-8, %3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 32 32' width='16' height='16' class='ibm-icons ibm-icons--error--filled' fill='%23bebebe'%3e%3cpath d='M6.95 26.66A14 14 0 0 0 26.67 6.93zM16 2A14 14 0 0 0 5.35 25.07L25.08 5.34A13.93 13.93 0 0 0 16 2z'/%3e%3c/svg%3e");
Copy link
Contributor

Choose a reason for hiding this comment

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

Double-checking - Do you think usage this technique should be only temporarily? Something that came up to my mind is build toolchain addition to generate Sass variables from SVG, but I guess only if we'd ship this code to stable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is just temporary, I think the we should continue with inline svgs as our standard for stable releases.

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

looks good! 👍

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

Changes look great!

@jeanservaas
Copy link
Collaborator

Text Input Feedback

Question: can we use a smaller line for the field, like just "Place holder" or "Type here" so it doesn't cut off?


Text Color

  • Hint text needs to be Gray 50 (right now it is at Gray 70)

  • "Typing" and "Finished" states are both Gray 100
    (right now it has an active "typing" state of Gray 100 and finished typing is at (Gray 70)

(See grabs)

hint_text

typing

finished_typing


Helper text

  • There is no helper text below the field, only a validation message (in the red). Helper text appears in a tool tip. (See grab) This is different from how Carbon does it now, so it won’t be a 1:1 relationship.

(See helper_text grab)

helper


Disabled

  • Can’t find it in the inspection, we need to use Plex Sans Regular (Font weight 400)— but it looks like you’re using the semi-bold here (font-weight:600), the font weight is thicker…

screen shot 2018-09-20 at 11 29 22 am


Vertical center

  • This is a niggling detail. Looks like the type is riding slightly low in the box (like 1px) Should be on vertical center

vertical_center

@alisonjoseph
Copy link
Member Author

alisonjoseph commented Sep 20, 2018

Thanks for the feedback @jeanservaas

http://carbon-dev-environment-optimistic-kangaroo.stage1.mybluemix.net/component/text-input--default

  • Text Color -- updated
  • Helper text -- I believe this will have to be addressed in the future since it is a html / ux change and we are trying to add css only updates right now. (however someone should be able to just add an icon with tooltip next to the label and it will work)
  • Disabled -- It's actually the same font weight (400) as the other text inputs, the color makes it look a little different in browser.
  • Vertical center -- added 1px padding-bottom

@alisonjoseph alisonjoseph merged commit b8bf8b8 into carbon-design-system:master Sep 21, 2018
@alisonjoseph alisonjoseph deleted the text-input-x branch September 21, 2018 14:14
@carbon-bot
Copy link
Contributor

🎉 This PR is included in version 9.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Experimental Component: Text Input
9 participants