adobe-photoshop / spaces-design

Adobe Photoshop Design Space
http://adobe-photoshop.github.io/
Other
852 stars 74 forks source link

Mixed Appearance panel shows controls for valid layers in selection #3662

Closed DivyaPrabhakar closed 8 years ago

DivyaPrabhakar commented 8 years ago

Fixes issues #3777, #3756, #3615, #3147, and #2457

Issue #3777:

Before:

After: I have made all disabled inputs use the specified CSS color for each stop @underlines-disabled. In the examples below, blend mode and opacity are enabled and the rest of the type settings are disabled, just to give an idea of the different states relative to one another. @placegraphichere thoughts?

Issue #3756

Opacity and Blend Mode Alignment Before:

After:

Color Picker Inputs Alignment Before:

After:

Radius Input Alignment: Before:

After:

Issue #3615 Before:

After:

Issue #3147 Before:

After:

Issue #2457 @placegraphichere Can I get your opinion about this too?

Before:

After:

DivyaPrabhakar commented 8 years ago

@volfied or @chadrolfs Can you make sure the now activated fields are the correct ones? Is there anything that should not be active? One thing I saw on master also is when two text layers are selected Mixed Font Styles is active but there are no options to choose from. Is that correct behavior?

ktaki commented 8 years ago

Hi @DivyaPrabhakar Let me chime in.

Found an issue. Create a rectangle and a text layer. Change the color of the rectangle. Select both layers. Now the color of rectangle in the panel is wrong. Color picker is not available when clicking on the panel for changing the color of the rect.

screen shot 2016-02-03 at 2 44 11 pm

When two text layers are selected, the current behavior looks right though more testing is needed.

screen shot 2016-02-03 at 2 49 49 pm

chadrolfs commented 8 years ago

A couple more things (leveraging Taki's screenshots above):

DivyaPrabhakar commented 8 years ago

@ktaki and @chadrolfs

1.) The appearance panel does not scroll anymore in the mixed state

2.) The blend modes and opacity are now aligned

3.) The color picker for vector shapes is fixed

iwehrman commented 8 years ago

It may well be inconvenient to implement this with our current component structure, but I definitely do not think showing a fill color swatch row AND a type color swatch row is the correct design when some type layers and some vector layers are selected. There should just be one swatch/blend-mode row, and the behavior should be intuitively uniform: the swatch is used to change the type AND vector colors simultaneously, as appropriate for the given selected layer. Why should the user care that these two swatches are wired up to different underlying actions? Instead it should be possible to change all the colors in a single operation.

DivyaPrabhakar commented 8 years ago

I think @iwehrman is right. Initially I was thinking that the current approach made sense because the user cases for choosing multiple layers at a time (ranked by priority) is 1.) To make a property change to all selected layers 2.) To move the selected layers on the canvas.

In the case that the user selects the layers to move them around, my initial hesitation was that we can't assume they want to apply a change to a property (like fill) to all the layers selected at once, so we need to provide the separated text color and vector fill options. But in that case, we already know the primary objective for the user is to change the placement of the layers, not change a property so it doesn't matter if we provide one or two different fill options anyway. Another thing I saw is that when the vector fill and text color are enabled, the stroke swatch is below the text. I think it should be paired with the vector fill swatch.

I will create a new branch off of this one and implement the one swatch approach for us to look at and decide.

chadrolfs commented 8 years ago

sounds good @DivyaPrabhakar, let @ktaki and I know when the new branch is ready and we'll give it a once over.

ktaki commented 8 years ago

@DivyaPrabhakar Toggle Fill seems not functional.

screen shot 2016-03-04 at 9 55 26 am
ktaki commented 8 years ago

The Sampler is broken in this branch. Make sure this isn't a side-effect of your change.

chadrolfs commented 8 years ago

Comments on new branch (mixed-appearance-refactoring-2):

Hitting Tab key after editing Typeface field in mixed selection state results in an error: screen shot 2016-03-04 at 11 39 44 am

chadrolfs commented 8 years ago

rotating a mixed selection changes the attributes but shouldn't mixed-rotate

DivyaPrabhakar commented 8 years ago

@ktaki and @chadrolfs -- I have updated the PR description with all the bugs that this PR should fix -- mainly UI fixes. Also the problems that you both identified before should @ktaki and @chadrolfs -- The problems that you both identified before should be fixed:

1.) Color swatch keeps the fill when the selections are rotated

2.) Sampler tool works

3.) Hitting Tab key after editing Typeface field in mixed selection state results in an error:Hitting Tab key after editing Typeface field in mixed selection state results in no error now.

chadrolfs commented 8 years ago

color picker is blinks mixed: blinky-cp

chadrolfs commented 8 years ago

interplay between the opacity in the color picker and the panel opacity is funky with a mixed selection that includes a pixel layer.

chadrolfs commented 8 years ago

changing the tracking fails: Action type.setTracking failed: Error: Photoshop returned an error when playing command number: 1 Photoshop code: -25920 Photoshop message: error: -25920 message: The command “Set” is not currently available.

chadrolfs commented 8 years ago

same thing with Alignment: Action type.setAlignment failed: Error: Photoshop returned an error when playing command number: 1 Photoshop code: -25920 Photoshop message: error: -25920 message: The command “Set” is not currently available.

ktaki commented 8 years ago

The issues with manipulating alpha value could be caused by the issue reported in #3786 in which the alpha value for the text layers is treated differently from that of shape layers.

ktaki commented 8 years ago

I did not find any other new issues.

DivyaPrabhakar commented 8 years ago

@chadrolfs I fixed the errors that were occurring on alignment and tracking and I fixed part of the opacity issue -- before if you used the slider, only text and vector layers would change opacity, but now all layers change opacity. Also the user can change opacity via the opacity input field i.e. type opacity = 30 and all the layers will adjust accordingly.

2 known problems still exist -- 1.) When the opacity slider is adjusted, the opacity input field does not reflect this change. This is because of the open bug @ktaki mentioned above, and we have decided not to fix that bug. 2.) The color swatch overlay goes to mixed state when the user rotates the layers. I know why this is happening and was working on the fix but I need a little help from @iwehrman or @pineapplespatula, and I have commented on that line and will wait for their feedback.

The rest of the issues both you and @ktaki found should have been addressed. I will let you know right when a decision on problem no. 2 is made. Sorry for the delay on this, it took me some time to narrow down the problem 2.

DivyaPrabhakar commented 8 years ago

@chadrolfs The rotation error should be fixed now!

chadrolfs commented 8 years ago

OK, I think this'll do it! Thanks for all your hard work on this. Ok to merge!

baaygun commented 8 years ago

Looks good to me, since it's also heavily tested, I saw no blaring errors.