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

Differences in online help pane toolbar #13684

Closed
steff456 opened this issue Sep 1, 2020 · 6 comments · Fixed by #13686
Closed

Differences in online help pane toolbar #13684

steff456 opened this issue Sep 1, 2020 · 6 comments · Fixed by #13686

Comments

@steff456
Copy link
Member

steff456 commented Sep 1, 2020

Problem Description

The toolbar of the Online Help pane has some differences in master and 4.x,

  1. First the buttons are in a complete different order
    Master
    image

4.x
image

  1. Placeholder text is different
    Master
    image

4.x
image

What is the expected output? What do you see instead?

The online help pane should look the same as 4.x

@goanpeca
Copy link
Member

goanpeca commented Sep 1, 2020

Hi, @steff456 thanks for the feedback.

  1. First the buttons are in a complete different order

This was a deliberate change and goes in line with the change in the Find in Files an Pylint plugin where the button for starting and stopping is the same since those actions are mutually exclusive. This change for Find and Pylint was discussed and agreed by @ccordoba12 many months ago.

Now, on why the button moved: Browsers (Like Chrome and Firefox) provide that interface, where the stop and refresh button are one and the same. Since this plugin is basically a "specialized" browser, it should provide an interface that is already familiar to users and that is why:

  • 2 buttons became 1
  • The location on the far right was updated to provide a familiar interface.

Chrome

Screen Shot 2020-09-01 at 15 59 07

Firefox

Screen Shot 2020-09-01 at 15 58 17

I would like to hear what @isabela-pf thinks of this?

  1. Placeholder text is different

I can restore the original text.

@steff456
Copy link
Member Author

steff456 commented Sep 2, 2020

This was a deliberate change and goes in line with the change in the Find in Files an Pylint plugin where the button for starting and stopping is the same since those actions are mutually exclusive.

Ok, we can keep the change of the order in the toolbar. Maybe we can unify the refresh button with the stop one, as the changes you already made in the Find in files and Pylint plugin.

I can restore the original text.

Thanks!

Please take into account that for this plugin migration the idea is to maintain as closely as we can the interface so then we can make the changes we are studying in meetings with Isabella and that's why some of my comments are more related to how things are looking in the 4.x branch.

@ccordoba12
Copy link
Member

ccordoba12 commented Sep 2, 2020

Since this plugin is basically a "specialized" browser, it should provide an interface that is already familiar to users

We discussed your proposed changes with the rest of team and agreed that they make sense.

However, please do not introduce any additional changes to the UX/UI of the plugins that have not been migrated yet to the new API. You're conflating the migration process with those changes, which are unrelated tasks.

Besides, you're introducing a lot of changes without discussing them with anyone else in the team, which not only makes our task of reviewing your PRs harder (because we need to verify very carefully what has changed between 4.x and master); but we also consider that's not the proper way to approach that task.

Instead, we're reviewing pane by pane with Isabella in weekly meetings with her, and deciding how to improve their UX/UI in a concerted manner.

@goanpeca
Copy link
Member

goanpeca commented Sep 2, 2020

Hi @steff456 and @ccordoba12 thanks for the feedback.

Ok, we can keep the change of the order in the toolbar. Maybe we can unify the refresh button with the stop one, as the changes you already made in the Find in Files and Pylint plugin.

Thanks for the understanding, but to clarify the unifying of the Search/Stop, Find/Stop buttons is something @ccordoba12 agreed from the beginning (4-5 months ago).

I just took a decision based on a similar change (already vetted for two other plugins). Moving the button (from the right to the left) was a necessary change because it would not have made much sense leaving the button to the far right, so that was just a consequence of unifying the buttons.

Please take into account that for this plugin migration the idea is to maintain as closely as we can the interface so then we can make the changes we are studying in meetings with Isabella and that's why some of my comments are more related to how things are looking in the 4.x branch.

Agreed, @steff456, and there have been several issues that appeared due to how the API changed how things were made, or some are just the result of having to rebase PRs that have stayed without revision for many months, and when rebasing them against master sometimes I did not check with 4.x.

@spyder-ide/core-developers please take into account that the new API completely restructures how the top buttons in a Pane are created and laid out, so replicating the exact same things that 4.x does, is not something that always makes sense technically. The case of unifying the buttons that were performing mutually exclusive actions is a good example of this.

@ccordoba12
Copy link
Member

ccordoba12 commented Sep 2, 2020

@goanpeca, we agreed with the rest of the team that for the next PRs on queue in the new API migration process (starting with the Pylint one) you need to leave their layout and UI/UX as close as possible, or even better the same, as the one currently displayed in 4.x.

In case that's not possible, you need to clearly justify the technical reasons that don't allow you to do that. As I said above, it's not part of this process to also introduce UI/UX changes. Instead, that's part of the project we're working on with Isabella.

@goanpeca
Copy link
Member

goanpeca commented Sep 2, 2020

@goanpeca, we agreed with the rest of the team that for the next PRs on queue in the new API migration process (starting with the Pylint one) you need to leave their layout and UI/UX as close as possible, or even better the same, as the one currently displayed in 4.x.

Sounds good.

In case that's not possible, you need to clearly justify the technical reasons that don't allow you to do that. As I said above, it's not part of this process to also introduce UI/UX changes. Instead, that's part of the project we're working on with Isabella

I have always technically justified why changes are needed and will continue to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants