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(select): prepend support on textfield & select #82

Merged
merged 1 commit into from
May 28, 2019
Merged

Conversation

i8ramin
Copy link
Contributor

@i8ramin i8ramin commented May 9, 2019

No longer breaking change. Decided to not touch the current with-icon implementation and create new styles to support prepend.

Main motivation (re #81)

148bf077b51e186b763e56e7423305df _Screen%20Shot%202019-05-09%20at%2011 05 05%20AM

Screenshots

Screen Shot 2019-05-10 at 10 31 16 AM

Screen Shot 2019-05-09 at 2 02 06 PM

Screen Shot 2019-05-09 at 2 02 52 PM

@adamraider
Copy link
Contributor

@i8ramin Interesting contribution, however, this will need to be audited by design. Can you please confer with Dana Ballasy about this change?

@adamraider
Copy link
Contributor

What is the breaking change for?

@i8ramin
Copy link
Contributor Author

i8ramin commented May 9, 2019

What is the breaking change for?

@adamraider, there are new wrapping div's that need to be added for the with-icon and with-prepend stuff to work. I'll point them out in the PR

No longer breaking change. with-icon behaves the same, but prepend still requires the wrapper div

@cmugla
Copy link

cmugla commented May 9, 2019

hmmm.. so the idea is you would prepend a select to the text field in order to solve for phone field?

Might be useful to provide an example of this specific use case (with a dummy select), since that is the reason for adding? like textfield with a select story

@cmugla
Copy link

cmugla commented May 9, 2019

doesn't look like disabled behaves the same way with -prepend
Screen Recording 2019-05-09 at 03 14 PM

@i8ramin
Copy link
Contributor Author

i8ramin commented May 10, 2019

doesn't look like disabled behaves the same way with -prepend
Screen Recording 2019-05-09 at 03 14 PM

That was a bad copy/paste job. It was missing the disabled prop. Fixed now

@i8ramin
Copy link
Contributor Author

i8ramin commented May 10, 2019

hmmm.. so the idea is you would prepend a select to the text field in order to solve for phone field?

Might be useful to provide an example of this specific use case (with a dummy select), since that is the reason for adding? like textfield with a select story

This is just the first step towards that end goal. Adding a phone select in there is gonna be tricky, so didn't want to create a huge PR. This one just makes it so we can "prepend" things to textfield & selects

@i8ramin
Copy link
Contributor Author

i8ramin commented May 10, 2019

@cmugla added a simple flag dropdown (no functionality, just visuals)

Screen Shot 2019-05-10 at 10 31 16 AM

@@ -140,5 +147,22 @@
color: rgba($ray-color-gray-60, 0.4);
}
}

&--simple {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new addition to ray-select to support things like the flag dropdown

Screen Shot 2019-05-10 at 10 34 27 AM

border-radius: 0;

&__input {
font-size: ($ray-field-label-size * 1.2);
Copy link
Contributor Author

@i8ramin i8ramin May 10, 2019

Choose a reason for hiding this comment

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

this is specifically to make the flags a bit larger .. not sure if we should set this font size here

@i8ramin i8ramin changed the title feat(component): prepend support on textfield & select feat(select): prepend support on textfield & select May 10, 2019
@adamraider
Copy link
Contributor

@i8ramin Is there any way to avoid the breaking change? We just released 1.0.0 lol

@i8ramin
Copy link
Contributor Author

i8ramin commented May 10, 2019

@i8ramin Is there any way to avoid the breaking change? We just released 1.0.0 lol

@adamraider its only breaking if you used with-icon support in textfield and selects. otherwise, everything works as expected.

No longer breaking changes. New wrapper div is only required for the new with-prepend support.

Copy link

@cmugla cmugla left a comment

Choose a reason for hiding this comment

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

This looks good to me. Please go through the chromatic failures

@i8ramin i8ramin force-pushed the rb-prepend branch 2 times, most recently from d18ca98 to 630a200 Compare May 24, 2019 17:54
@i8ramin i8ramin requested a review from cmugla May 24, 2019 18:11
BREAKING CHANGE: textfields and selects that have icons (and not prepends) need to be wrapped in an
extra div with a class name of "#{$ray-class-prefix}#{$class}__wrapper"

re #81
@@ -44,11 +46,15 @@
border-radius: 2px;

[dir='rtl'] & {
position: unset;
margin-left: $ray-field-h-spacing;
right: calc(100% - (2 * #{$ray-field-h-spacing}));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a slight change in how we handle the arrow on RTL. Instead of unsetting position, we calculate it so that the behavior is similar to how it is done with LTR

Copy link

@cmugla cmugla 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 to me

Copy link
Contributor

@adamraider adamraider left a comment

Choose a reason for hiding this comment

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

LGTM, so long as this was approved by Dana feel free to merge

@i8ramin
Copy link
Contributor Author

i8ramin commented May 28, 2019

Screen Shot 2019-05-28 at 3 59 35 PM

@i8ramin i8ramin merged commit ea1f00e into master May 28, 2019
@delete-merged-branch delete-merged-branch bot deleted the rb-prepend branch May 28, 2019 20:00
adamraider pushed a commit that referenced this pull request May 29, 2019
textfields and selects that have icons (and not prepends) need to be wrapped in an
extra div with a class name of "#{$ray-class-prefix}#{$class}__wrapper"

re #81
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.

3 participants