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

Expose ProgressEvent to ServiceWorker #308

Merged
merged 4 commits into from
Jan 26, 2021
Merged

Conversation

Rashika101
Copy link
Contributor

@Rashika101 Rashika101 commented Jan 23, 2021

Fixes #305
"Expose ProgressEvent to ServiceWorker"

  • At least two implementers are interested (and none opposed):
    • "Gecko"
    • "WebKit"
  • Tests are written and can be reviewed and commented upon at:
    • Tested through IDL.
  • Implementation bugs are filed:
    • Chrome: N/A
    • Firefox: N/A
    • Safari: N/A

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

"Expose ProgressEvent to ServiceWorker"
@saschanaz
Copy link
Member

Hey, thanks, the patch looks perfect! A few nits for the description, could you:

  • Modify the title to "Expose ProgressEvent to ServiceWorker" to show the intention of this patch?
  • Remove a space in [x ] so that it can be [x]. You'll see it magically becoming a checkbox!
  • Add Fixes #305 at the top line. Doing so will show this patch in the issue so that other's won't duplicate your work 😉

@Rashika101
Copy link
Contributor Author

Is that okay?
Yes,The checkbox has been created..
But in 1st point title modification what is that?Isn't similar?

@saschanaz
Copy link
Member

You'll see the Edit button next to the Open with button, please click it and you can edit the title 😊

@saschanaz
Copy link
Member

saschanaz commented Jan 23, 2021

BTW, you need to sign the Participation Agreement for your contribution, please go here and do the required steps. Sorry for the unexpected paperwork 😬

@saschanaz
Copy link
Member

After that, during workdays someone will review this once more and merge it. Thank you for your hard work 😍

@Rashika101 Rashika101 changed the title Update xhr.bs "Expose ProgressEvent to ServiceWorker" Jan 24, 2021
@Rashika101
Copy link
Contributor Author

Thank you
@saschanaz Modified the title!!
Explain a little more on participation agreement ,what are the steps i need to take in that agreement...
When I am submitting ,"Invalid entity name name " is coming..

@saschanaz
Copy link
Member

Entity name? I think you are supposed to fill only the "If signing as an Individual" form 👀 (unless you are doing this as a part of your job).

@saschanaz
Copy link
Member

And thank you for setting the right title! It would be even better without the quotes.

@Rashika101 Rashika101 changed the title "Expose ProgressEvent to ServiceWorker" Expose ProgressEvent to ServiceWorker Jan 24, 2021
@Rashika101
Copy link
Contributor Author

Thank You @saschanaz
Quotes removed...
Signed the Participation agreement.
When my PR will merge?
and still my participation showing pending..

@saschanaz
Copy link
Member

Per this page it needs to be verified by the admins, which will happen probably next week. Hold tight 😉

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks! Could you change this to Window,Worker instead? Worker is a shorthand for all three and we generally use that when applicable.

@annevk
Copy link
Member

annevk commented Jan 25, 2021

Also, you should feel free to add your name to the Acknowledgments section.

@Rashika101
Copy link
Contributor Author

@annevk
Is there any changes i have to make?

@Rashika101
Copy link
Contributor Author

@saschanaz Thank You for your guidance!!
Is there any thing else that i have to do?

@annevk
Copy link
Member

annevk commented Jan 25, 2021

@Rashika101 yeah, instead of Window,DedicatedWorker,SharedWorker,ServiceWorker you want Window,Worker. And you can also add your name to the Acknowledgments section at the bottom of the document if you wish.

@saschanaz
Copy link
Member

saschanaz commented Jan 25, 2021

To reduce confusion: Worker is a shorter equivalent of DedicatedWorker,SharedWorker,ServiceWorker. We want to make the code short as possible to make it more manageable, and thus [Exposed=(Window,Worker)] is preferred here. Sorry for confusing you 🙏

Feel free to ping me when you need any more help 💕

@Rashika101
Copy link
Contributor Author

@saschanaz @annevk
I did the changes as per your guidance. A new patch and pull request has been created...
Did I had done it correctly? Please recheck it.
Thanks!

@saschanaz
Copy link
Member

saschanaz commented Jan 26, 2021

Hmm, you don't have to create new patch, you can edit existing patch by Files changed -> "..." button (next to xhr.bs) -> Edit file. But that also works 👀

@Rashika101
Copy link
Contributor Author

Should I do it again now in this patch?
As you say...

@saschanaz
Copy link
Member

I'd say please give it a try 😁

@Rashika101
Copy link
Contributor Author

Done.....Is that visible?

@saschanaz
Copy link
Member

Yup, perfectly splendid!

@Rashika101
Copy link
Contributor Author

Thank You...Any change required in title?

@saschanaz
Copy link
Member

No, the patch still does the same work, just in a shorter way 😀

BTW, have you considered adding your name to Acknowledgments? We totally want to list the contributors who gave valuable patch(es) and you also deserve it! It's not a requirement, so feel free to ignore it.

@Rashika101
Copy link
Contributor Author

Yes, i want to add name...How to do that?

@saschanaz
Copy link
Member

It's same, in the same file search "Acknowledgments" and you'll see several names ordered in alphabetical way. Please add your name there 😉

@Rashika101 Rashika101 requested a review from annevk January 26, 2021 10:17
@Rashika101
Copy link
Contributor Author

Excuse me.....feeling stupid
Didn't get acknowledgement...

@saschanaz
Copy link
Member

saschanaz commented Jan 26, 2021

Oh, it's "Acknowledgments" (it lacks "e" between "legd" and "ments", English is weird 😄)

@Rashika101
Copy link
Contributor Author

Sorry.....
Where do I search it?

@saschanaz
Copy link
Member

saschanaz commented Jan 26, 2021

In the xhr.bs file, the one you just edited. Make sure you tap Control+F (Or Command+F if you are on macOS) after you click the text editor or you won't see anything.

@saschanaz
Copy link
Member

saschanaz commented Jan 26, 2021

Oh I mean,

  1. Files changed -> "..." button (next to xhr.bs) -> Edit file
  2. Search "Ackno" (by Control+F or Command+F)
  3. Add your name, in the alphabetical order 😉

@Rashika101
Copy link
Contributor Author

I did it in the same way. But nothing happens.

@saschanaz
Copy link
Member

Could you explain what you saw on the screen when you tried it? Did you fail to find the names?

@Rashika101
Copy link
Contributor Author

When I search "Ackno" nothing comes up..

@saschanaz
Copy link
Member

Did you click the text editor before searching? It works in a bit weird way, it may not come up if you didn't.

(It's sad that we couldn't ship FindText API to make this straightforward.)

@Rashika101
Copy link
Contributor Author

Is there any other way to do so?
I am trying but nothing happens.

@saschanaz
Copy link
Member

Hmm, alternatively you can just scroll down to line number 1911 (the number you see at the left of the text editor) and it's there.

image

@Rashika101
Copy link
Contributor Author

Oh,Thank You so much.
I did it.
Anything else i need to do.

@saschanaz
Copy link
Member

Good! Could you move your name before Robin Berjon so that the list can be alphabetically ordered? (so that Rashika comes before Robin)

@Rashika101
Copy link
Contributor Author

Oh excuse me.
Done.

@saschanaz
Copy link
Member

Okay, now everything is perfect and we can just wait for the verification for your participation form. Thank you! 🎉🎉

@Rashika101
Copy link
Contributor Author

Thank you. Until we can work on other issues.?

@saschanaz
Copy link
Member

I guess no good first issue anymore in this repository. I think issues in whatwg generally require some knowledge of web standards than others (this one was luckily exceptional 😁) , so you may want to try other repositories too. @annevk may have some thought, though.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks! I looked through some of the remaining good first issues within the WHATWG and it seems they require a bit more experience indeed.

@annevk annevk merged commit 8088cc5 into whatwg:main Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Expose ProgressEvent to ServiceWorker
3 participants