Update selectors for querying events from mozilla #195
Conversation
It seems the page https://developer.mozilla.org/en-US/docs/Web/Events has changed. This commit Updates the selector to reflect this.
| @@ -135,7 +135,7 @@ func main() { | |||
|
|
|||
There was a problem hiding this comment.
I suspect it's a good idea to update nameMap above so the generated names have proper casing:
There was a problem hiding this comment.
Can you please expand on what you mean by proper casing? The goal of this PR is just to fix the selector to properly query the page. I believe other changes can be more meaningful in a separate PR.
There was a problem hiding this comment.
Sure. This PR currently adds some new functions to event package, like:
DevicechangeMessageerrorSlotchange- etc.
According to Go style conventions (https://golang.org/s/style#mixed-caps and https://golang.org/s/style#initialisms), identifiers should use MixedCaps casing. So the new functions should be named:
DeviceChangeMessageErrorSlotChange- etc.
nameMap provides a mapping from HTML attribute names to proper Go style names. By adding new entries there, you can ensure the functions that are added as part of this PR follow Go style.
If your goal is not to add any new functions to event package as part of this PR, you'll need to make changes. That's not currently the case.
There was a problem hiding this comment.
Thanks for the explanation. I will add the mapping, it will be weird just submitting the changes in the generator file without the updated generated file.
|
@shurcooL I updated the nameMap. Hope I didn't miss anything. |
event/generate.go
Outdated
| @@ -30,6 +30,7 @@ func main() { | |||
| "animationend": "AnimationEnd", | |||
| "animationiteration": "AnimationIteration", | |||
| "animationstart": "AnimationStart", | |||
| "appinstalled": "AppInstalled", | |||
There was a problem hiding this comment.
I'm not sure if "App" should be considered a full word, or if it's short for "Application". If the former, this is good. If the latter, this should be mapped to "ApplicationInstalled", similar to how "dblclick" is mapped to "DoubleClick" below (rather than to "DblClick").
Given that the event is described as (emphasis mine):
The
appinstalledevent of the Web Manifest API is fired when the browser has successfully installed a page as an application.
I'm leaning slightly towards ApplicationInstalled.
I'll defer to @slimsag on this.
There was a problem hiding this comment.
I agree, ApplicationInstalled is preferred here.
| @@ -376,7 +382,7 @@ func DurationChange(listener func(*vecty.Event)) *vecty.EventListener { | |||
|
|
|||
| // Emptied is an event fired when the media has become empty; for example, this | |||
| // event is sent if the media has already been loaded (or partially loaded), | |||
| // and the load() method is called to reload it. | |||
| // and the load() method is called to reload it. | |||
There was a problem hiding this comment.
We should figure out what changed here. This may be the MDN using another weird space unicode character, and if so, we should swap it out in the generator with a plain ASCII space character.
There was a problem hiding this comment.
Probably copying https://github.com/gernest/vecty/blob/5f529e0c4616b201de3fc21c4c8873c92862bf89/elem/generate.go#L158-L160 into this generator would do the trick.
There was a problem hiding this comment.
We should figure out what changed here.
I already checked. MDN changed a non-breaking whitespace (U+00A0) for a normal whitespace (U+0020).
Copying desc = strings.Replace(desc, "\u00a0", " ", -1) might still be desirable if you really want to eliminate those non-breaking whitespaces, in case they come up in the future or elsewhere.
However, I think having non-breaking whitespace in documentation isn't a big deal. I don't see a reason to actively avoid them.
There was a problem hiding this comment.
Got it. This change LGTM as-is then.
Unrelated to this change, but I do want to avoid non-breaking whitespace in documentation because it easily leads to changes like this where there is uncertainty about why a line has changed.
| // ScriptProcessorNode is ready to be processed. | ||
| // | ||
| // https://developer.mozilla.org/docs/Web/Events/audioprocess | ||
| func AudioProcess(listener func(*vecty.Event)) *vecty.EventListener { |
There was a problem hiding this comment.
It seems like this has gotten removed by the generator even though it is still a valid event (not deprecated etc). We'll need to figure out / fix why this got removed before we can merge this.
There was a problem hiding this comment.
@slimsag apologies for the late feedback, I was in a hospital , I feel better now. I t seems the cause is the .icon-thumbs-down-alt class see https://github.com/gopherjs/vecty/pull/195/files#diff-7f46ba42e4ad2574c9cbe6836be77d61L140 I don't know why the logic is there. Maybe the docs added that icon for some reason.
There was a problem hiding this comment.
Sorry to hear you were not feeling well! Hope you're feeling better.
I just had a chance to look at this, and it looks like the problem is just more inconsistency on the MDN -_-
The audioprocess event is not marked as deprecated, but the data type it events AudioProcessingEvent is deprecated. Obviously that is silly because that means both are deprecated, so we're doing the right thing here by removing this (since we don't keep deprecated items in here). Just another reason to solve #136 soon.
Codecov Report
@@ Coverage Diff @@
## master #195 +/- ##
==========================================
- Coverage 47.53% 47.46% -0.07%
==========================================
Files 3 3
Lines 648 651 +3
==========================================
+ Hits 308 309 +1
- Misses 299 300 +1
- Partials 41 42 +1
Continue to review full report at Codecov.
|
|
@shurcooL I fixed the nameMap for |
It seems the page https://developer.mozilla.org/en-US/docs/Web/Events
has changed. This commit Updates the selector to reflect this.