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

Improve Recent repositories settings form #3769

Conversation

pmiossec
Copy link
Member

@pmiossec pmiossec commented Jun 18, 2017

Configure repositories popup:

  • Being able to see the repository selected in the lists
  • Being able to do an action on multiple repositories at the same time
  • Resize column size to prevent bad display (when destination list was initially empty or changing shortening strategy)
  • Display 'Anchored' repositories in bold
  • Add buttons to move repositories between more/less recent repositories
  • Add a button to be able to remove all the deleted repositories from the list of recent repositories

Repositories list:

  • Always display a separator between pinned and all recent repositories
  • Add '...' to 'Configure this menu' menu entry because it will open a popup (follow windows practices)
  • Add an icon in the menu for pinned repositories

Before:
image

image

After:
image

image

@pmiossec
Copy link
Member Author

There is a last thing that I want to change but I want your point of view first...

I never really understood that notion of "Most recent repositories" and "Less recent repositories" because that's not compatible with fact that the user could modify these lists.

Shouldn't we call them something like "Preferred recent repositories" and "Other recent repositories"?

@jbialobr
Copy link
Member

Remove deleted repositories from the list of recent repositories

I would not do that automatically, you can add a button to trigger this action. The reason is that people can keep their repos on removal devices or subst drives. Maybe the candidates for removal should be printed in red.

@jbialobr
Copy link
Member

Shouldn't we call them something like "Preferred recent repositories" and "Other recent repositories"?

Hard to say as the first group is an union of preferred repos and most recent repos.
"Preferred recent repositories" sounds to me more like intersection of preferred and recent repos.
In a case when there are no repos pinned to the most recent group, they are just most recent repositories.

@pmiossec pmiossec force-pushed the recent_repositories_settings_improvements branch from 1bddc57 to 1e5836d Compare June 18, 2017 12:11
@RussKie
Copy link
Member

RussKie commented Jun 18, 2017

Put "Ok" button on the left of the "Cancel" one

Genuinely interested why? There are so many dialogs and forms, yet I can't really see any pattern in how they have their buttons.
For example "Process" dialog has buttons in the following order [Abort] [OK]...

Remove deleted repositories from the list of recent repositories

Agree with @jbialobr, for the same reason I display warning icons on my dashboard PR

Display 'Anchored' repositories in bold

What is an 'anchor' and how it is get used? And how do you anchor it?
Regardless I would probably do it by using an icon rather than making it bold.
The same icon can then be used everywhere else - the dashboard, the repo dropdown etc - thus providing the visual continuity of experience.

Add buttons to move repositories between more/less recent repositories

The intent is good but I find the execution uninspiring, sorry. The buttons just don't belong in that spot.
May be like this?
image
Though even that doesn't look too appealing...
The buttons should also be context aware - become enabled/disabled depending on available selection or lack of.
Also have you added the same images to the context menu?

@RussKie
Copy link
Member

RussKie commented Jun 18, 2017

WRT: most/less recent repositories
It is something I struggle to fully comprehend as well. Is this something that really needed to be carried forward?
Can't we maintain a list of repos in order of their access. The repo used the last sits at the top, the repo used before it sinks below it. Every time a repo is being opened/switched to - the list of recent repos gets updated...

Then the whole UI becomes simpler - in this form as well as in the dashboard, as well as a lot of code can be put to rest - reducing the tech debt and maintenance burden.
This will potentially allow for making this dialog the place to manage how repositories are presented and categorised. Maybe even re-purpose/re-use it for categories management (as part of my dashboard PR #3693)

@RussKie
Copy link
Member

RussKie commented Jun 18, 2017

And one more thing - please squash it before merging in.

@pmiossec
Copy link
Member Author

I would not do that automatically, you can add a button to trigger this action. The reason is that people can keep their repos on removal devices or subst drives. Maybe the candidates for removal should be printed in red.

done.

Hard to say as the first group is an union of preferred repos and most recent repos.
"Preferred recent repositories" sounds to me more like intersection of preferred and recent repos.
In a case when there are no repos pinned to the most recent group, they are just most recent repositories.

I don't really get it but I won't touch it....

Put "Ok" button on the left of the "Cancel" one

Genuinely interested why? There are so many dialogs and forms, yet I can't really see any pattern in how they have their buttons.

Because that's the Windows guidline. And yes, that should be fixed on other forms to be consistent....

Remove deleted repositories from the list of recent repositories

Agree with @jbialobr, for the same reason I display warning icons on my dashboard PR

@RussKie Fixed, even before you post your very sympathic message...

Display 'Anchored' repositories in bold

What is an 'anchor' and how it is get used? And how do you anchor it?
Regardless I would probably do it by using an icon rather than making it bold.

I thought about that but it's a little harder to do and because I didn't found a statisfying icon and I'm not a designer, I decided not to go with that and that my improvement will be enough for the few case this form should be used. Everyone made efforts to improve things... but I'm waiting for your PR.

Add buttons to move repositories between more/less recent repositories

The intent is good but I find the execution uninspiring, sorry. The buttons just don't belong in that spot.
May be like this?

I tried also but it should require to redesign most of the form what I'm not enough confident to do. But I wait for your contribution, too....

Though even that doesn't look too appealing...

Indeed, critics are easy....

The buttons should also be context aware - become enabled/disabled depending on available selection or lack of.

That is already done...

Also have you added the same images to the context menu?

No, good idea, I could do it.

It is something I struggle to fully comprehend as well. Is this something that really needed to be carried forward?

I don't have the pretension to question choices that have been made. I just tried to improve the bad experience I felt each time I used this form (I didn't remember each time that there is a contextual menu, what is anchor, ....). This is my simple contribution to make it more understandable.

And one more thing - please squash it before merging in.

No, I don't think so. Each commit is an improvement on it's own and I highly prefer that history merged instead of a big commit. But the maintainer/merger will choose what he prefers now that it is easy to do thanks to github....

@EbenZhang
Copy link
Contributor

No, I don't think so. Each commit is an improvement on it's own and I highly prefer that history merged instead of a big commit. But the maintainer/merger will choose what he prefers now that it is easy to do thanks to github....

Agree, as long as the commit is logic.
It's hard to know the reason of the change in a large commit when investigating a bug.

@jbialobr
Copy link
Member

Can't we maintain a list of repos in order of their access. The repo used the last sits at the top, the repo used before it sinks below it. Every time a repo is being opened/switched to - the list of recent repos gets updated...

I don't like when items in the menu constantly change theirs position.

Then the whole UI becomes simpler - in this form as well as in the dashboard, as well as a lot of code can be put to rest - reducing the tech debt and maintenance burden.
This will potentially allow for making this dialog the place to manage how repositories are presented and categorised. Maybe even re-purpose/re-use it for categories management (as part of my dashboard PR #3693)

I think this menu should share the same display logic as the dashboard proposed in #3693.

@jbialobr
Copy link
Member

And one more thing - please squash it before merging in.

No, I don't think so. Each commit is an improvement on it's own and I highly prefer that history merged instead of a big commit. But the maintainer/merger will choose what he prefers now that it is easy to do thanks to github....

I reacted with thumb up to the @EbenZhang comment, but I want to state it explicitly:
The more details we know about the reason standing behind a commit the easier is to make a judgement if the change is correctly implemented. Keeping commits small and atomic helps to achieve that state.
So, many small commits are welcome.

@RussKie
Copy link
Member

RussKie commented Jun 20, 2017

So, many small commits are welcome.

I was going to suggest the same.

@RussKie
Copy link
Member

RussKie commented Jun 20, 2017

I just tried to improve the bad experience I felt each time I used this form

This is great!

I don't have the pretension to question choices that have been made.

...And this isn't great.

...but I'm waiting for your PR.
But I wait for your contribution, too....

Unless I am misreading it, you expect me to come and fix things after you? I find it bewildering...

Indeed, critics are easy....

There is a distinction between "criticism" and "feedback". The former is to say something is bad, the latter is to say something isn't right and suggest a possible alternative.

No need to feel defensive when there is feedback. UIs are hard, and sometimes they take a lot of effort and iterations to get right, for them to feel natural, easy to use and look appeasing.
It is ok to make an effort and fail - this way we learn 😃 ...as long as we stand up and keep on walking.

How about left-aligning the buttons like this:
image

@RussKie
Copy link
Member

RussKie commented Jun 20, 2017

I don't like when items in the menu constantly change theirs position.

The repo which you're currently working with will be at the top... But that was a random idea anyway not a formed proposal.

I think this menu should share the same display logic as the dashboard proposed in #3693.

Totally! Are you/we open to reconsidering the current implementations?
The UX should be unified between repo history, categories, sort/grouping order and the navigation. Right now it is very fragmented and disjointed.

What is an 'anchor' and what does it get used for anyway?

@jbialobr
Copy link
Member

Are you/we open to reconsidering the current implementations?

I would like to. I think that repository dropdown could be populated with repositories grouped in categories - the same categories as on dashboard, with uncategorised including and preserving the order of categories and repos set for the dashboard. For me the most recent repos group acts as a 'favorites' category with a constant set of pinned repos.

An anchor is used to pin a repo to most/less recent repositories group.

@@ -62,6 +65,8 @@ private void SetShorteningStrategy(string strategy)
throw new Exception("Unhandled shortening strategy: " + strategy);
}

private static Font AnchorFont = new Font(new ListViewItem().Font, FontStyle.Bold);
Copy link
Member

Choose a reason for hiding this comment

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

Can you make it non-static and derive from the actual list font? Is it a performance optimization?

@jbialobr jbialobr added this to the 2.51 milestone Jun 21, 2017
@RussKie RussKie removed this from the 2.51 milestone Dec 13, 2017
@pmiossec pmiossec force-pushed the recent_repositories_settings_improvements branch from 1e5836d to 77d42fa Compare December 13, 2017 22:35
@sharwell sharwell added the 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed label Jan 23, 2019
@gerhardol gerhardol removed the 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed label Feb 7, 2019
@GintasS
Copy link
Collaborator

GintasS commented Aug 6, 2021

This is an old PR and should be closed or set to a draft. @RussKie

@pmiossec pmiossec force-pushed the recent_repositories_settings_improvements branch from 77d42fa to f56a86c Compare January 17, 2023 15:23
@ghost ghost assigned pmiossec Jan 17, 2023
@pmiossec pmiossec force-pushed the recent_repositories_settings_improvements branch 3 times, most recently from 247c656 to 9f44117 Compare January 20, 2023 09:59
@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Mar 11, 2023
@ghost ghost added the 💤 status: no recent activity The issue is stale label Apr 10, 2023
@ghost
Copy link

ghost commented Apr 10, 2023

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 30 days. It will be closed if no further activity occurs.

@pmiossec pmiossec force-pushed the recent_repositories_settings_improvements branch from 75c174f to 2b7d269 Compare April 20, 2023 13:48
@ghost ghost removed 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity 💤 status: no recent activity The issue is stale labels Apr 20, 2023
@pmiossec pmiossec force-pushed the recent_repositories_settings_improvements branch from 2b7d269 to 145fc4e Compare April 20, 2023 13:52
@gerhardol
Copy link
Member

@pmiossec rebase this on master or target 4.1?

I do not want to delay 4.1 any more...

@mstv
Copy link
Member

mstv commented Apr 20, 2023

I do not want to delay 4.1 any more...

👍

@pmiossec
Copy link
Member Author

@pmiossec rebase this on master or target 4.1?

I have removed all the commits that raised the discussion to keep only the little improvement that makes using this form a little more easier.
And I would like to see a start of improvement included in v4.1
I don't go very often in this screen but every time I do, the experience is really bad...

The code is finished and the branch is in good state but I had to go and didn't had the time to update the PR. I will do tomorrow morning...

After that, you will whatever you want with it hoping it won't delay the release...

@pmiossec pmiossec force-pushed the recent_repositories_settings_improvements branch 4 times, most recently from 29515dc to 004a776 Compare April 21, 2023 08:23
@pmiossec pmiossec marked this pull request as draft April 21, 2023 08:39
@pmiossec pmiossec force-pushed the recent_repositories_settings_improvements branch from 004a776 to 432e5a7 Compare April 30, 2023 07:45
by jumping directly to values that has an effects i.e 0 to 30
(values between 1 and 30 will have the same effect than 30)

Prevent the user to have to click 30 times on a up or down buttons
to see an effect.
@pmiossec pmiossec force-pushed the recent_repositories_settings_improvements branch from ff2b1df to 92a1d0e Compare April 30, 2023 14:25
@pmiossec pmiossec closed this Apr 30, 2023
@pmiossec pmiossec deleted the recent_repositories_settings_improvements branch April 30, 2023 16:53
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