Skip to content

[FancyZones Editor] New UX for the FZ editor.#9325

Merged
enricogior merged 63 commits intomasterfrom
users/niels9001/fz-editorUXoverhaul
Jan 27, 2021
Merged

[FancyZones Editor] New UX for the FZ editor.#9325
enricogior merged 63 commits intomasterfrom
users/niels9001/fz-editorUXoverhaul

Conversation

@SeraphimaZykova
Copy link
Copy Markdown
Contributor

@SeraphimaZykova SeraphimaZykova commented Jan 27, 2021

Summary of the Pull Request

This PR introduces a brand new UX for the FZ editor created by @niels9001

New features:

  • Ability to duplicate layouts
  • New UX that simplifies the creation and selection of (new) layouts.
  • Theming support (dark/light and all high contrast themes).
  • UI can now be fully controlled with tab/arrow keys
  • Monitor selection now supports keyboard (for selection)
  • Accessibility improvements.
  • Other minor enhancements (e.g. scrollable list).

Editor
image

New layout dialog
image

Edit template layout
image

Edit custom layout
image

Light theme
image

Quality Checklist

Contributor License Agreement (CLA)

A CLA must be signed. If not, go over here and sign the CLA.

/// </summary>
public partial class CanvasEditorWindow : EditorWindow
{
private int _offset = 100;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add a comment to explain what is this offset referring to?

@enricogior
Copy link
Copy Markdown
Contributor

@niels9001 @SeraphimaZ
you did an amazing work!!! 🥇

@htcfreek
Copy link
Copy Markdown
Collaborator

I agree with @enricogior. Thank you for this nice UI.

Model.PropertyChanged += OnGridDimensionsChanged;

int zoneCount = _data.ZoneCount;
for (int i = 0; i <= zoneCount; i++)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't we do i < zoneCount instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, but with a little fix in GridData. In fact, ZoneCount represents the biggest used index in the grid, so the name is misleading. I'll update the code so that it would represent exactly what it means.

@htcfreek
Copy link
Copy Markdown
Collaborator

htcfreek commented Jan 27, 2021

@enricogior
Can we do layout based review too or do we only review the functional things to merge it for 0.31?

public App()
{
DebugModeCheck();
// DebugModeCheck();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is it commented out? Should we remove it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We still want to enable debug mode for stand alone debugging, we can revisit this in 0.33 or 0.35.

}
}

_zoneCount = maxIndex + 1;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Before ZoneCount was equal to maxIndex not it is equal to maxIndex+1. Does it mean that indexing changed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, indexing wasn't changed. I explained above the reason for it, ZoneCount was a misleading name.

// The Microsoft Corporation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: redundant using directive

@Jay-o-Way
Copy link
Copy Markdown
Collaborator

Jay-o-Way commented Jan 27, 2021

Looking good! 😁

May I make a few suggestions?
@ Editor: top part - I see you left away the resolution of the monitors. Is that for a specific reason? I believe that people like to see it.
@ New layout dialog - Top title mentions "type", but immediately below that there is an input for the name. Maybe "New layout" for the title and "Name" and "Type" labels would be a bit better? Are there any forbidden characters for the name? (to prevent the settings file from malfunctioning)
@ Edit (template) layout - A NumberBox for "Number of zones" and "Show space" and "Distance to highlight" would be better. I would mention "px" with the last two. Also we could combine the controls for Show space around zones?

image

@niels9001
Copy link
Copy Markdown
Collaborator

niels9001 commented Jan 27, 2021

Looking good! 😁
May I make a few suggestions?
@ Editor: top part - I see you left away the resolution of the monitors. Is that for a specific reason? I believe I've seen that people like to see it.

This has been moved into a tooltip, so hovering over / on focus would show the tooltip. Let's look at the feedback to see if it needs to be visible at all times.

@ New layout dialog - Top title mentions "type", but immediately below that there is an input for the name. Maybe "New layout" for the title and "Name" and "Type" labels would be a bit better?

Agree. I think the dialog title should just be "Create new layout".

@ Edit (template) layout - Maybe a NumberBox for "Number of zones" and "Show space" and "Distance to highlight" would be better. I would mention "px" with the last two aswel. Also we could combine the controls for Show space around zones?

The edit dialog probably needs some (minor) moving around of controls. Could you open an issue for that so it's track-able :)?

xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
mc:Ignorable="d"
Title="Window1" Height="450" Width="800"
Title="Window1" Height="450"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

"Window1"?

@crutkas
Copy link
Copy Markdown
Member

crutkas commented Jan 27, 2021

@enricogior, the installer build got stuck on a heart beat, i'm good to go if you are. I kicked off another one but it is only 1 minute into the actual compile.

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.

8 participants