Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Add support for percentage based RGB colors to the inline color editor #2212

Merged
merged 4 commits into from
Apr 25, 2013

Conversation

DennisKehrig
Copy link
Contributor

Fix for #2050

@ghost ghost assigned RaymondLim Nov 26, 2012
@DennisKehrig
Copy link
Contributor Author

Assigning @RaymondLim because he filed the issue

Hey Raymond, would you mind testing this? Thanks!

@RaymondLim
Copy link
Contributor

@DennisKehrig Your files are out-of-sync and I had to manually duplicate your changes in order to test the conversion to percentage rgb format.

Nice work! I wonder where you get this version of tinycolor-min.js. When I locate tinycolor in Github, I only found our current version v0.5.

I don't think your changes to the layout in the template are ok. @GarthDB and @njx Can you guys take a look at @DennisKehrig changes? He added one button and increase the width of the input text field. It's nice to have one more button to do the conversion, but we may need to have a better layout though.

@DennisKehrig
Copy link
Contributor Author

Hey @RaymondLim, thanks for the review!

I rebased my changes on the current master.
For tinycolor, see bgrins/TinyColor#9

You're right about the UI changes, it looks weird with the aside so far to the side. In my opinion it should start right next to the sliders, above the buttons.
I increased the size of the input field to make strings like "rgba(100%, 100%, 100%, 0.65)" fit.

@GarthDB Do you want to work on that or should I take a shot at it?

@GarthDB
Copy link
Member

GarthDB commented Nov 28, 2012

I'll work on it.

Sent from my iPhone

On Nov 28, 2012, at 4:33 AM, Dennis Kehrig notifications@github.com wrote:

Hey @RaymondLim, thanks for the review!

I rebased my changes on the current master.
For tinycolor, see bgrins/TinyColor#9

You're right about the UI changes, it looks weird with the aside so far to the side. In my opinion it should start right next to the sliders, above the buttons.
I increased the size of the input field to make strings like "rgba(100%, 100%, 100%, 0.65)" fit.

@GarthDB Do you want to work on that or should I take a shot at it?


Reply to this email directly or view it on GitHub.

@DennisKehrig
Copy link
Contributor Author

Rebased onto master

@DennisKehrig
Copy link
Contributor Author

Hey @GarthDB, do you have any updates for me on this?

@peterflynn
Copy link
Member

@GarthDB @DennisKehrig NJ suggested we simplify this so it essentially has no UI changes -- opening the color editor on a percentage color would show none of the existing 3 color format buttons as selected, and as you drag the sliders the number would stay in percentage format. If you click a button to change the format, then there's no way to get back to percentage format other than manually editing the code. That seems like a reasonable tradeoff between supporting this less-common format vs. keeping the UI clean & simple. Thoughts?

@GarthDB
Copy link
Member

GarthDB commented Mar 6, 2013

I think that would work. I would think that if a user is using the visual color editor they may not be so concerned with whether it is percentage or not.

Sent from Mailbox for iPhone

On Wed, Mar 6, 2013 at 12:20 PM, Peter Flynn notifications@github.com
wrote:

@GarthDB @DennisKehrig NJ suggested we simplify this so it essentially has no UI changes -- opening the color editor on a percentage color would show none of the existing 3 color format buttons as selected, and as you drag the sliders the number would stay in percentage format. If you click a button to change the format, then there's no way to get back to percentage format other than manually editing the code. That seems like a reasonable tradeoff between supporting this less-common format vs. keeping the UI clean & simple. Thoughts?

Reply to this email directly or view it on GitHub:
#2212 (comment)

@DennisKehrig
Copy link
Contributor Author

@peterflynn I think it's a good compromise!

@DennisKehrig
Copy link
Contributor Author

@RaymondLim UI changes removed. Note that strings like rgba(100%, 100%, 100%, 0.5) are now too long for the input box.

@RaymondLim
Copy link
Contributor

Sorry Dennis. You need to resolve the conflict. After you resolve it, I'll land it to master for this sprint.

Conflicts:
	src/extensions/default/InlineColorEditor/InlineColorEditor.js
@DennisKehrig
Copy link
Contributor Author

Hey @RaymondLim, thanks for the heads up! I merged master into the branch. I needed to change the unit tests a bit, please check if that is okay.

@RaymondLim
Copy link
Contributor

Looks good. Merging.

RaymondLim added a commit that referenced this pull request Apr 25, 2013
Add support for percentage based RGB colors to the inline color editor
@RaymondLim RaymondLim merged commit f605b47 into adobe:master Apr 25, 2013
@DennisKehrig
Copy link
Contributor Author

Thank you :)

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.

4 participants