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

Option to control actions alignment in search results #63457

Merged

Conversation

IllusionMH
Copy link
Contributor

Fixes #61532

This PR adds new search.actionsPosition option that can force search result action (replace, replace all, dismiss) to be always displayed at the right edge of the panel.

Other change - count badge for files and folders matches are displayed at the end of match and visible on hover. Only negative aspect of later change is shift of badge in really narrow search block when actions become visible on hover. Since count badge doesn't have click handlers its shift shouldn't be so bad (in comparison with any actionable element).

@roblourens looks like I've removed changes introduces in your commit. Could you please describe what issues this commit fixed or give me a link to the issue. I will check that this PR doesn't regress there.

Also I haven't found test related to search results badge/actions layout etc. Are there tests that I should update in this PR?

Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the PR! And for cleaning up the CSS 👍

@roblourens roblourens merged commit e8db92f into microsoft:master Nov 27, 2018
@roblourens roblourens added this to the November 2018 milestone Nov 27, 2018
'search.actionsPosition': {
type: 'string',
enum: ['auto', 'right'],
default: 'auto',
Copy link

Choose a reason for hiding this comment

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

why is this not right?
that would make a ton more sense than auto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I personally prefer right I decided to preserve behavior that was default in 1.29 because my issue (filled even before 1.29 release) got 0 👍 and only two supporting comments.

In any way it may be discussed and changed if maintainers are OK with it.

Copy link

Choose a reason for hiding this comment

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

I've already opened a PR to be address this. Thanks for making this configurable! It's been plaguing me last few days.

Copy link
Member

Choose a reason for hiding this comment

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

We've discussed this a bunch already, I'm not going to change the default because 'right' doesn't make sense to me for wide views on large monitors. The setting is here for your to customize however you want.

@joaomoreno
Copy link
Member

This caused #63834 so I will revert it for now.

joaomoreno added a commit that referenced this pull request Nov 27, 2018
Fixes #63834

Revert "Cleanup new setting description"

This reverts commit b995518.

Revert "Merge pull request #63457 from IllusionMH/search-actions-align-option-61532"

This reverts commit e8db92f, reversing
changes made to 4201f51.
@IllusionMH
Copy link
Contributor Author

@joaomoreno thank you for notification.
I was able to reproduce it "search.actionsPosition": "right" or "search.actionsPosition": "auto" and view is "narrow" (less than 1000px wide).
Are there other cases that I should check?

I will submit new PR today.

roblourens added a commit that referenced this pull request Nov 27, 2018
@IllusionMH IllusionMH deleted the search-actions-align-option-61532 branch December 10, 2018 23:42
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change of Dismiss action button position in Search panel
4 participants