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

[FancyZones] Scrollable Editor #7516

Closed
wants to merge 4 commits into from
Closed

[FancyZones] Scrollable Editor #7516

wants to merge 4 commits into from

Conversation

davidegiacometti
Copy link
Collaborator

@davidegiacometti davidegiacometti commented Oct 24, 2020

Summary of the Pull Request

Added scrolling to Fancy Zones Editor

PR Checklist

Info on Pull Request

Tested on:

  • 1920x1080
  • 1024x768 SizeToContent="Height" no more needed

FZ1
FZ2

Validation Steps Performed

Test FancyZones editor with scrolling

@niels9001
Copy link
Contributor

#6899 addresses this as well. But this PR would be an easier merge.

@davidegiacometti
Copy link
Collaborator Author

#6899 addresses this as well. But this PR would be an easier merge.

Didn't know about FancyZones UI/UX review 😅

@htcfreek
Copy link
Collaborator

Merging this could conflict with #6562.

@enricogior
Copy link
Contributor

Hi @davidegiacometti
thank you very much for your contribution.

We should wait before merging this PR for a couple of reasons.
We don't want to revert previous changes for the editor UI for small screens, it's OK to add scrolling but it should still look like this
image
and not like this since it present a discoverability issue
image

We are also in the process of redesign the UI #1032
Changes to the current UI will require a new accessibility assessment, so instead of doing it twice (for your changes and then again for the new UI) let's work together to update/improve the UI after #6562 is merged (that also introduces changes to the UI).

I'm going to convert this PR to draft for now, OK?

@enricogior enricogior marked this pull request as draft October 26, 2020 09:01
@ghost ghost added the FancyZones-Editor Issue revolving the FancyZone Editor label Nov 7, 2020
@enricogior
Copy link
Contributor

@davidegiacometti
we just merged the editor multi-monitor support.
The UI is different when multiple monitors are present, so make sure that adding the scroll bar doesn't change how the UI is rendered in that case.
If you don't have a multi monitor configuration, you can simulate it passing arguments in debug mode to the editor.
In case let me know if you need any hint on how to do it.

@davidegiacometti
Copy link
Collaborator Author

@enricogior I have fixed scrolling for both sizes but I think the UI can be improved further.
I have updated the first post with the result.
Also the Close button appear only when there is more than one monitor. Not sure if a bug or a feature 😄

@davidegiacometti davidegiacometti marked this pull request as ready for review November 17, 2020 21:06
@enricogior
Copy link
Contributor

@davidegiacometti

I think the UI can be improved further

This is the next step for the UI: #6899

Also the Close button appear only when there is more than one monitor. Not sure if a bug or a feature 😄

That is by design, because the single monitor UI has not been changed, and applying a layout closes the editor.
On multi-monitor, applying a layout doesn't close the editor, so a dedicated Close button is needed.

@enricogior
Copy link
Contributor

@davidegiacometti
we need to revisit these changes because I don't think this is an improved UI:

image

It doesn't have a visible scroll bar and for a first time user it's not immediately clear there are two more templates (Grid and Priority Grid).
On large screens is not very useful to have a fixed window size, it's not a good usage of the screen estate, we should adjust the windows size based on the space available.
Given we will not accept more changes for 0.27 after this Friday, it might be better to work with @niels9001 and join forces to add scroll bar to his upcoming PR for the updated UI for the December release.

@davidegiacometti
Copy link
Collaborator Author

@enricogior now the scroll bars are visibles.
I agree that the fixed size is not optimal on large screens, than feel free to close the PR without merging and release this in December with the new UI.
image

@enricogior enricogior marked this pull request as draft November 19, 2020 15:49
@enricogior
Copy link
Contributor

@davidegiacometti
thanks for fixing the scrollbars visibility.
I've changed the PR to draft since now it's too late to makes other changes and test them for 0.27.
Niels is starting to work on the revised UI, so we can wait for his work and then decide what can be reused from this PR, in case we will close it and open a new one.

@davidegiacometti
Copy link
Collaborator Author

@enricogior I think this can be closed since @niels9001 is working on #8148

@enricogior
Copy link
Contributor

@davidegiacometti
OK. Thanks.

@enricogior enricogior closed this Nov 23, 2020
@davidegiacometti davidegiacometti deleted the issue-738 branch February 4, 2021 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FancyZones-Editor Issue revolving the FancyZone Editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants