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

Add missing OffsetX and OffsetY events to MouseEventArgs. #20478

Merged
merged 3 commits into from
Apr 14, 2020

Conversation

Valks
Copy link
Contributor

@Valks Valks commented Apr 2, 2020

Added missing OffsetX and OffsetY values to the MouseEventArgs

Addresses #15440

Notes:

  • (WIP) I haven't been able to build the artifacts (build itself passes) locally so I'm not sure if this is all the changes required.
  • Couldn't find any tests to append. If there are tests for events some advise would be appreciated.

@Valks Valks requested a review from SteveSandersonMS as a code owner April 2, 2020 23:22
@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Apr 2, 2020
@dnfclas
Copy link

dnfclas commented Apr 2, 2020

CLA assistant check
All CLA requirements met.

@captainsafia
Copy link
Member

(WIP) I haven't been able to build the artifacts (build itself passes) locally so I'm not sure if this is all the changes required.

You'll also need to update the reference assemblies. You can read more about how to do this in this docs page.

Couldn't find any tests to append. If there are tests for events some advise would be appreciated.

ATM, there are no unit tests that check for attributes on browser events. I would recommend adding an E2E test to validate this change.

Run the BasicTestApp test project and navigate to the Mouse events cases. You'll see that there is a test component to validate various mouse events.

image

This is the same component that the E2E tests validate against.

@Valks
Copy link
Contributor Author

Valks commented Apr 6, 2020

Thanks for the feedback.

I've taken a look using the BasicTestApp and it's working as expected.

Taking a look at the E2E tests they all seem to check if the event triggers but there's no tests to check the output values.

@captainsafia
Copy link
Member

captainsafia commented Apr 8, 2020

Taking a look at the E2E tests they all seem to check if the event triggers but there's no tests to check the output values.

I think it is fine if we don't add test cases for this particular scenario if we don't already validate individual keys. It's not a particularly meaningful validation anyways. Let me know if you think otherwise @dotnet/aspnet-blazor-eng.

@mkArtakMSFT mkArtakMSFT added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 8, 2020
@captainsafia
Copy link
Member

Refing this to #8241.

@captainsafia captainsafia added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 10, 2020
@ghost
Copy link

ghost commented Apr 10, 2020

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@SteveSandersonMS
Copy link
Member

Looks totally reasonable to me. Thanks, @Valks!

@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Apr 13, 2020
@captainsafia
Copy link
Member

@Valks Since this PR contained a code change that modified the public interface, it had to undergo an API review. Luckily, it has passed API review and is approved for merge.

There's a few things to do before merging:

  1. There's now a merge conflict in the blazor.server.js file. To resolve this build issue, you will need to rebase or merge against master, then run:
$ cd src/Components/Web.JS
$ npm run build

To regenerate the built JavaScript asset with both your changes and the changes that have been pushed into master.

Also, it looks like the build failure in this PR is due to a flaky test. Pushing anew commit with the changes from #2 should resolve this.

Let me know if you have follow-up questions.

@Valks Valks force-pushed the extend-mouseevents branch from 1137ea9 to 7d14edc Compare April 13, 2020 21:24
@Valks
Copy link
Contributor Author

Valks commented Apr 14, 2020

Hi @captainsafia, thanks for all your help.

Just wanting to understand should I create a commit adding kvm.cmd from #2? If I've misunderstood please elaborate because it feels a little strange adding code from a 6-year-old pull request.

@captainsafia
Copy link
Member

Just wanting to understand should I create a commit adding kvm.cmd from #2? If I've misunderstood please elaborate because it feels a little strange adding code from a 6-year-old pull request.

Typo on my part. That was meant to say #1. The merge commit should've triggered a rebuild. This is ready for merging now. Thanks for making the change and addressing the follow-ups.

@captainsafia captainsafia merged commit 75c7797 into dotnet:master Apr 14, 2020
@Valks Valks deleted the extend-mouseevents branch April 14, 2020 19:39
@Xaeco
Copy link

Xaeco commented Apr 29, 2020

Hi everyone, firstly I'd like to say a big THANK YOU for the great work on Blazor and DotNet Core, I'm loving it!
Second, what preview/nightly builds do I need to download to get the OffsetX and OffsetY working in my app.

Thanks
Mat

@captainsafia
Copy link
Member

The fix has not been released yet but will be released as part of the preview in May.

@KieranDevvs
Copy link

@captainsafia Has this been released yet as I'm currently on the latest version of Blazor and do not see Offset X/Y within the MouseEventArgs class?
image

@captainsafia
Copy link
Member

@KieranDevvs What preview have you installed? I am not able to repro the issue on 5.0.0-preview5.

@captainsafia
Copy link
Member

Is it safe to assume these properties will be included in the .NET 5 release, but not before?

Correct. This will be shipped in .NET 5. If you use one of the previews, you should be able to play around with this change.

@captainsafia
Copy link
Member

@jackbond I see. Can you open a new issue with this question? Include a description of your goal and a repro if possible. The team will address it during triage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants