[ATL-959] Remove settings icon from sidebar #209

fstasi · 2021-03-12T08:48:20Z

A gear icon is shown prominently in the side bar

This PR removes the gear icon from the sidebar

Jira Task
Github Issue: fixes #15

fstasi · 2021-03-12T08:49:27Z

arduino-ide-extension/src/browser/contributions/settings.ts

+
+        export const CLOSE: Command = {
+            id: 'arduino-settings-close',
+            label: 'Close Preferences...',
+            category: 'Arduino'
+        }


I don't think this approach is really necessary as the Dialog provides a Promise that, when resolved (usually with close(), closes the UI.

I agree. We do not need a command for it. Especially, a command that is available from the Commnad Palette. Note: if you add a label to the command, users can execute it from the Commad Palette.

thanks for the tip about the label!

fstasi · 2021-03-12T09:00:50Z

arduino-ide-extension/src/browser/settings.tsx

+            <div className='flex-line'>
+                <button className='theia-button shrink' onClick={this.openExtendedSettings}>Extended Settings</button>
+            </div>


@kittaakos This is just a draft button in the Arduino settings panel. When clicked it opens the "theia" settings in a tab, and - supposedly - closes the current Arduino settings panel.

I don't really like the way I close this dialog, as I don't think I need an extra command, as the SettingsComponent shouldn't be aware it's being used in a dialog.

In my opinion a better approach should be sending a message to the parent (Widget) stating that the extended settings are being opened. The Widget should communicate somehow to the Dialog to call the .close().

This communication between
Dialog > Widget > Settings Component is the real pain here. Do you have ideas on how to address this in an elegant way>?

as I don't think I need an extra command

I agree. Let's do it without a command.

fstasi · 2021-03-12T09:02:42Z

arduino-ide-extension/src/browser/settings.tsx

        </div>;
    }

+    protected openExtendedSettings = (e: React.MouseEvent<HTMLElement>) => {
+      this.props.commands.executeCommand(CommonCommands.OPEN_PREFERENCES.id);
+      this.props.commands.executeCommand('arduino-settings-close');


Even if I defined CommonCommands.CLOSE_PREFERENCES I'm not even able to use it here, as it causes a loop in the dependencies.

If we go the "commands path" we should move these preference commands to another file, but my understanding is we don't do that usually.

kittaakos · 2021-03-12T09:07:09Z

arduino-ide-extension/src/browser/contributions/settings.ts

+
+        export const CLOSE: Command = {
+            id: 'arduino-settings-close',
+            label: 'Close Preferences...',
+            category: 'Arduino'
+        }


I agree. We do not need a command for it. Especially, a command that is available from the Commnad Palette. Note: if you add a label to the command, users can execute it from the Commad Palette.

kittaakos · 2021-03-12T09:07:29Z

arduino-ide-extension/src/browser/settings.tsx

@@ -13,6 +13,8 @@ import { FileService } from '@theia/filesystem/lib/browser/file-service';
 import { ThemeService } from '@theia/core/lib/browser/theming';
 import { MaybePromise } from '@theia/core/lib/common/types';
 import { WindowService } from '@theia/core/lib/browser/window/window-service';
+import { CommandRegistry } from "@theia/core";


Please use single quotes. I have to add linter for it. Thanks!

nice, we don't use prettier or editorconfig, correct? do we plan to use it in the future?

kittaakos · 2021-03-12T09:09:49Z

arduino-ide-extension/src/browser/settings.tsx

+            <div className='flex-line'>
+                <button className='theia-button shrink' onClick={this.openExtendedSettings}>Extended Settings</button>
+            </div>


as I don't think I need an extra command

I agree. Let's do it without a command.

kittaakos · 2021-03-12T09:11:13Z

arduino-ide-extension/src/browser/settings.tsx

@@ -681,12 +693,17 @@ export class SettingsWidget extends ReactWidget {
    @inject(WindowService)
    protected readonly windowService: WindowService;

+    @inject(CommandRegistry)


FYI: There is the CommandService API, which's sufficient for this case, as you want to only execute a command. No need to change, just wanted to share this with you.

👍 I'm gonna check the difference between the CommandService and the CommandRegistry

kittaakos · 2021-03-12T09:29:41Z

I have two questions regarding the UX, I guess you have already discussed it with the designers:

What should happen when the user makes a few changes in the dialog and opens the advanced settings: do we save the changes before opening the advanced settings editor and closing the dialog or not?
If we have to persist the changes before opening the advanced settings, what should happen when the user set invalid settings. Does the dialog show the validation errors before opening the advanced settings and closing itself?
What should happen when the advanced editor is already open and the user opens it?

Thanks!

FYI: if the user changes anything in the advanced settings editor, it is very likely that the change goes to the workspace settings. There will be a new .theia/settings.json file in the sketch folder.

ubidefeo · 2021-03-12T20:01:37Z

What should happen when the user makes a few changes in the dialog and opens the advanced settings: do we save the changes before opening the advanced settings editor and closing the dialog or not?

any change to the Arduino preferences before the advanced preferences should be saved

If we have to persist the changes before opening the advanced settings, what should happen when the user set invalid settings. Does the dialog show the validation errors before opening the advanced settings and closing itself?

which setting would be invalid? for instance characters in the scaling percentage?

What should happen when the advanced editor is already open and the user opens it?

as far as I understand that advanced settings tab is just a GUI rendering of a setting file, and by trying to open it twice from the gear icon it will just receive focus, not open into another tab

FYI: if the user changes anything in the advanced settings editor, it is very likely that the change goes to the workspace settings. There will be a new .theia/settings.json file in the sketch folder.

excellent point. I have to say that while for basic users that .theia folder would never even show up since it's hidden and I personally don't have a problem with advanced users having it there, but it breaks the sketch paradigm by introducing hidden files.
I think we should just bite the bullet and accept that you can have such things. @alranel was the one proposing to remove it in favour of a temporary path not to break the Sketch format, so let's see what his point is about this.

Thank you for now 👍🏼

fstasi · 2021-03-15T10:48:47Z

What should happen when the user makes a few changes in the dialog and opens the advanced settings: do we save the changes before opening the advanced settings editor and closing the dialog or not?

any change to the Arduino preferences before the advanced preferences should be saved

If we have to persist the changes before opening the advanced settings, what should happen when the user set invalid settings. Does the dialog show the validation errors before opening the advanced settings and closing itself?

which setting would be invalid? for instance characters in the scaling percentage?

What should happen when the advanced editor is already open and the user opens it?

as far as I understand that advanced settings tab is just a GUI rendering of a setting file, and by trying to open it twice from the gear icon it will just receive focus, not open into another tab

We had a chat @giannicolab and I about the "open extended preferences from standard preferences" and how friendly this flow is to the user.
Having the "extended preferences" button in the "quick" preferences opens up a number number of unclear scenarios (cleverly pointed out by @kittaakos ) that are somehow the direct consequence of a bad design choice in the first place.

@giannicolab is suggesting to have 2 menu items, such as "quick preferences" and "extended preferences", rather than having a button in the quick preferences that closes a modal and radically change how the user should interact with the settings. At the moment I think his approach is the best for the user, removing noise from the quick settings (no more extended button) and having to interaction flows which are distinct and separated.

@ubidefeo what is your opinion on this?

kittaakos · 2021-03-15T12:19:05Z

which setting would be invalid? for instance characters in the scaling percentage?

Yes. Or a completely invalid path pointing to the sketchbook.

any change to the Arduino preferences before the advanced preferences should be saved

@fstasi, if the changes have to be saved, we need accept() here:

arduino-ide/arduino-ide-extension/src/browser/settings.tsx

Line 762 in 0ac6030

this.widget.onCloseDidRequest(() => this.close())

Improved preference Dialog UI

ubidefeo

LGTM
I tested that the advanced settings helper came up with the shortcut.
we're good

fstasi requested a review from kittaakos March 12, 2021 08:48

fstasi commented Mar 12, 2021

View reviewed changes

fstasi force-pushed the atl-959--move-extended-pref branch from b4d382f to ba60402 Compare March 12, 2021 09:06

kittaakos reviewed Mar 12, 2021

View reviewed changes

fstasi force-pushed the atl-959--move-extended-pref branch from ba60402 to 8cb5398 Compare March 12, 2021 10:45

fstasi force-pushed the atl-959--move-extended-pref branch from 8cb5398 to 0ac6030 Compare March 15, 2021 08:24

fstasi changed the title ~~[draft] move settings from sidebar to arduino pref panel~~ [ATL-959] Move settings from sidebar to arduino pref panel Mar 15, 2021

move settings from sidebar to arduino pref panel

f7fb744

Improved preference Dialog UI

fstasi force-pushed the atl-959--move-extended-pref branch from 0ac6030 to f7fb744 Compare March 16, 2021 14:12

fstasi changed the title ~~[ATL-959] Move settings from sidebar to arduino pref panel~~ [ATL-959] Remove settings icom from sidebar Mar 16, 2021

fstasi changed the title ~~[ATL-959] Remove settings icom from sidebar~~ [ATL-959] Remove settings icon from sidebar Mar 16, 2021

fstasi marked this pull request as ready for review March 16, 2021 14:19

ubidefeo approved these changes Mar 16, 2021

View reviewed changes

fstasi merged commit e90fa27 into main Mar 17, 2021

fstasi deleted the atl-959--move-extended-pref branch March 17, 2021 09:52

per1234 mentioned this pull request Apr 3, 2021

No settings button (gear icon) and other #298

Closed

per1234 added topic: code Related to content of the project itself type: enhancement Proposed improvement labels Oct 28, 2021

per1234 mentioned this pull request Aug 30, 2022

Document how to access the advanced settings #1367

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ATL-959] Remove settings icon from sidebar #209

[ATL-959] Remove settings icon from sidebar #209

fstasi commented Mar 12, 2021 •

edited

Loading

fstasi Mar 12, 2021 •

edited

Loading

kittaakos Mar 12, 2021

fstasi Mar 12, 2021

fstasi Mar 12, 2021 •

edited

Loading

kittaakos Mar 12, 2021

fstasi Mar 12, 2021

kittaakos Mar 12, 2021

kittaakos Mar 12, 2021

fstasi Mar 12, 2021

kittaakos Mar 12, 2021

kittaakos Mar 12, 2021

fstasi Mar 12, 2021

kittaakos commented Mar 12, 2021

ubidefeo commented Mar 12, 2021

fstasi commented Mar 15, 2021 •

edited

Loading

kittaakos commented Mar 15, 2021

ubidefeo left a comment

[ATL-959] Remove settings icon from sidebar #209

[ATL-959] Remove settings icon from sidebar #209

Conversation

fstasi commented Mar 12, 2021 • edited Loading

fstasi Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fstasi Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittaakos commented Mar 12, 2021

ubidefeo commented Mar 12, 2021

fstasi commented Mar 15, 2021 • edited Loading

kittaakos commented Mar 15, 2021

ubidefeo left a comment

Choose a reason for hiding this comment

fstasi commented Mar 12, 2021 •

edited

Loading

fstasi Mar 12, 2021 •

edited

Loading

fstasi Mar 12, 2021 •

edited

Loading

fstasi commented Mar 15, 2021 •

edited

Loading