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

Fixed Scrolling through the history in the color picker editor with a mouse wheel #33551

Conversation

Fefedu973
Copy link
Contributor

@Fefedu973 Fefedu973 commented Jun 28, 2024

Summary of the Pull Request

Fixed Scrolling through the history in the color picker editor with a mouse wheel

PR Checklist

Detailed Description of the Pull Request / Additional comments

I added a mousewheel event listener on the HistoryColors ListView, then I added two functions in the ColorEditorView.xaml.cs to handle the Event, it checks the number of color in the history and handles the scroll control accordingly

Validation Steps Performed

I tested it, it works

This is my first pull request so idk if i've done it well enough...

@Fefedu973
Copy link
Contributor Author

@microsoft-github-policy-service agree

@abmprottoy
Copy link

abmprottoy commented Jun 29, 2024

Thank you very much for your pull request. Addressing this issue, which has been persistent for several months, is greatly appreciated.

I have run the changes, and they seem to be working as intended.

Desktop.2024.06.29.-.23.27.42.01.mp4

I would like to offer some feedback for consideration:

  • The function name might be more descriptive and concise if changed to HistoryColors_OnMouseWheelScroll

  • Adding a code comment using the <summary> tag above the function would enhance readability and understanding. Describe your mechanism concisely. Use second person voice. e.g. We added/We then...

  • Could you please provide more details regarding the other two commits that involve removing files? Understanding their purpose would be beneficial.

Thank you again for your contribution and effort in resolving this longstanding issue.

@Fefedu973
Copy link
Contributor Author

Okay I'll do that !
Concerning the two removed files they were automatically generated by visual studio and I forgot to remove them before pushing to my GitHub do I had to remove them afterwards but as you can see in the commit summary there is no deletion from the main source

@Fefedu973
Copy link
Contributor Author

@abmprottoy it's done !

@abmprottoy
Copy link

Thanks for the updates. The build seems to fail on two Tests. Could you please debug the issues? If not, let's wait for someone's expertise on this.

@Fefedu973
Copy link
Contributor Author

Thanks for the updates. The build seems to fail on two Tests. Could you please debug the issues? If not, let's wait for someone's expertise on this.

It works fine with me I haven't changed anything that can make tests failed in the build logs I saw that it was related to markdown so maybe it failed to fetch the change log during build

@abmprottoy
Copy link

@crutkas could you please look into this or assign someone so we can move with this to the next release?

@Fefedu973
Copy link
Contributor Author

anyone ?

@Fefedu973
Copy link
Contributor Author

so not in 0.82 i guess...

@abmprottoy
Copy link

abmprottoy commented Jul 4, 2024

so not in 0.82 i guess...

Unfortunately, yes. In the meantime, could you please add the "Needs-Review" label to this?
Also please update the PR description appropriately. Test checkbox should be left unticked as there's no Unit Testing added for this function. This PR also doesn't affect localization as there's no user facing string that needs modification for this.

As for the review, we still need one approving review from someone with write access which I lack.

I really hope this gets merged because the issue affects me too.

@Fefedu973
Copy link
Contributor Author

Ok !

@Fefedu973
Copy link
Contributor Author

i can't add labels

@Fefedu973
Copy link
Contributor Author

@htcfreek could you please add the need-review tag pls ?

@htcfreek htcfreek added the Needs-Review This Pull Request awaits the review of a maintainer. label Jul 6, 2024
@crutkas
Copy link
Member

crutkas commented Jul 9, 2024

so not in 0.82 i guess...

sorry, we had already signed off on 0.82 when this PR came in. We're actively working through our backlog of PRs.

@crutkas
Copy link
Member

crutkas commented Jul 10, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jaimecbernardo
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution!

@Fefedu973
Copy link
Contributor Author

Nice thanks!

@jaimecbernardo jaimecbernardo added Good to Merge and removed Needs-Review This Pull Request awaits the review of a maintainer. labels Jul 23, 2024
@htcfreek
Copy link
Collaborator

@jaimecbernardo
You marked this as good to merge. But sibce that happened Github locked merging. Don't know why.

And I am wondering about the temporary .csproj file. (Why is this part of the PR?)

@jaimecbernardo
Copy link
Collaborator

@jaimecbernardo You marked this as good to merge. But sibce that happened Github locked merging. Don't know why.

And I am wondering about the temporary .csproj file. (Why is this part of the PR?)

@htcfreek , I see no csproj in Files changed for this PR. What do you mean?

@jaimecbernardo
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@htcfreek
Copy link
Collaborator

@jaimecbernardo You marked this as good to merge. But sibce that happened Github locked merging. Don't know why.

And I am wondering about the temporary .csproj file. (Why is this part of the PR?)

@htcfreek , I see no csproj in Files changed for this PR. What do you mean?

My mistake. Looked at a older commit. Is fixed with on of the last cimmits.

@jaimecbernardo jaimecbernardo merged commit 63625a1 into microsoft:main Jul 25, 2024
13 checks passed
@Fefedu973
Copy link
Contributor Author

Nice

@Fefedu973
Copy link
Contributor Author

Idk what happend but on the last version my fix isn't working anymore. Someone broke it lol

@htcfreek
Copy link
Collaborator

Idk what happend but on the last version my fix isn't working anymore. Someone broke it lol

@Fefedu973
Can you please open a new issue referencing this fix. Thank you.

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

Successfully merging this pull request may close these issues.

5 participants