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

[Settings][Fix][Image Resizer ] Unused text box when selecting custom percent in new settings #4344

Merged
2 commits merged into from
Jun 19, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jun 16, 2020

Summary of the Pull Request

  • Added flags to hide and disable the "x" icon and the Hieght checkbox when unit is set to percentage.

Question:

  • Should we order by unit to group the percentage sizes so that the table looks consistent?
    image

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

  • Toggle the unit dropdown of the Image Sizes control to see if changes are applied

@ghost ghost requested a review from niels9001 June 16, 2020 22:27
@ghost ghost linked an issue Jun 16, 2020 that may be closed by this pull request
@htcfreek
Copy link
Collaborator

Is it possible to hide the second number input instead of the first? I think it is better to have the number near the value type.

@ghost
Copy link
Author

ghost commented Jun 16, 2020

Is it possible to hide the second number input instead of the first? I think it is better to have the number near the value type.

Sure let me do that.

Is it possible to hide the second number input instead of the first? I think it is better to have the number near the value type.

This is how it would look. What do you think?
image

@htcfreek
Copy link
Collaborator

Another Idea would be to make the first field bigger and center the number if percentage is choosen.

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 16, 2020

If it's possible we should split the settings visually:

<name>    <fit>    <value> x <value> <type>    [Delete]
<name>    <fit>              <value> <type>    [Delete]
<name>    <fit>    <value> x <value> <type>    [Delete]

@ghost
Copy link
Author

ghost commented Jun 16, 2020

If it's possible we should split the settings visually:

<name>    <fit>    <value> x <value> <type>    [Delete]
<name>    <fit>              <value> <type>    [Delete]
<name>    <fit>    <value> x <value> <type>    [Delete]

Let me play around with xaml and explore all the suggestions. I will post screenshots so we can evaluate and compare which one would optimize UX.

@htcfreek
Copy link
Collaborator

@laviusmotileng-ms
Please look at #2813.

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 16, 2020

I will post screenshots so we can evaluate and compare which one would optimize UX.

Let is discuss this in the main issue or #2813 and not in the PR. Otherwise the PR becomes to big and overloaded.

@ghost
Copy link
Author

ghost commented Jun 16, 2020

@laviusmotileng-ms
Please look at #2813.

Thats a good call out. issue #3015 is higher on the priority list. I wanted to focus on it on this PR. Issue #2813 focuses more on UX enhancements but does overlap with this to some extent.

Copy link
Contributor

@niels9001 niels9001 left a comment

Choose a reason for hiding this comment

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

@laviusmotileng-ms I'd personally would have set Visibility to Collapsed - we then only need 1 property (since it's hidden and thus not interactable). That would require replacing the StackPanel with a Grid with various Columns.

It's basically the same outcome, just a different approach so nothing blocking. Feel free to merge this in, looks good to me :)

@htcfreek
Copy link
Collaborator

@laviusmotileng-ms
How does it look at the moment? Have you something after the screenshot?

@ghost
Copy link
Author

ghost commented Jun 19, 2020

@laviusmotileng-ms I'd personally would have set Visibility to Collapsed - we then only need 1 property (since it's hidden and thus not interactable). That would require replacing the StackPanel with a Grid with various Columns.

It's basically the same outcome, just a different approach so nothing blocking. Feel free to merge this in, looks good to me :)

Collapsed doesn't work. It deletes the element from the UI including space it takes up causing the elements from the right to shift to the left. While changing the opacity and disabling it allows us to still keep that space.

@ghost
Copy link
Author

ghost commented Jun 19, 2020

@laviusmotileng-ms
How does it look at the moment? Have you something after the screenshot?

they're on the way.

@ghost
Copy link
Author

ghost commented Jun 19, 2020

@laviusmotileng-ms
How does it look at the moment? Have you something after the screenshot?

they're on the way.

let us solve this issue separately.

@ghost ghost merged commit 1653654 into master Jun 19, 2020
@htcfreek
Copy link
Collaborator

@laviusmotileng-ms
How does it look at the moment? Have you something after the screenshot?

they're on the way.

let us solve this issue separately.

What do you mean with issue?

@crutkas crutkas deleted the user/lamotile/remove_unwanted_boxes branch June 25, 2020 20:59
This pull request was closed.
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.

Image Resizer - unused text box when selecting custom percent in new settings
2 participants