[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

Begin migrating archive.js to vanilla JavaScript #9022

Merged
merged 18 commits into from
Jul 17, 2023
Merged

Begin migrating archive.js to vanilla JavaScript #9022

merged 18 commits into from
Jul 17, 2023

Conversation

ulisseshen
Copy link
Contributor
@ulisseshen ulisseshen commented Jul 12, 2023 •
edited by parlough
Loading

Contributes to #8889

Presubmit checklist

@google-cla
Copy link
google-cla bot commented Jul 12, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

parlough
parlough requested changes Jul 12, 2023
View reviewed changes
Copy link
Member
@parlough parlough left a comment

Choose a reason for hiding this comment

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

Thanks so much for working on this and opening a pull request. I'm excited to take a look!

However, I won't be able to review the PR until you sign the CLA with the email you committed with as outlined in the bot's comment. Let us know if you have issues signing it.

@atsansone atsansone added cla: no Contributor has not signed the Contributor License Agreement review.await-update Awaiting Updates after Edits labels Jul 12, 2023
@ulisseshen
Copy link
Contributor Author

@parlough I didn't know it was necessary to sign with Google CLA, however it's already done.

Should this be put on draft while i'ts still in progress?

@parlough
Copy link
Member

I didn't know it was necessary to sign with Google CLA, however it's already done.

Thanks for signing it!

Should this be put on draft while it's still in progress?

Draft is used to signify you are still working on the specific PR and aren't ready for review. Are you ready for a review?

@parlough parlough self-requested a review July 12, 2023 16:44
@ulisseshen
Copy link
Contributor Author

Are you ready for a review?

I think that this file is not completely migrate to pure js. I have migrate only 3 functions and I'll ocntinue working until I finish It. Also can you give me a guidance on what I can do.

@parlough
Copy link
Member
parlough commented Jul 12, 2023 •
edited
Loading

How about we keep this PR to migrating just those few functions. That way, it's easier to review and we can land improvements incrementally. That tends to be easier and safer. Could change the pull request title to something like "Begin migrating archive.js to vanilla JavaScript".

Then once this PR lands you can follow-up with a new pull request migrating more code if you'd like.

@parlough parlough removed cla: no Contributor has not signed the Contributor License Agreement review.await-update Awaiting Updates after Edits labels Jul 12, 2023
@parlough parlough self-assigned this Jul 12, 2023
@parlough parlough added the review.tech Awaiting Technical Review label Jul 12, 2023
@ulisseshen ulisseshen changed the title Feature/migrate vanillajs Begin migrating archive.js to vanilla JavaScript Jul 12, 2023
@parlough
Copy link
Member
parlough commented Jul 14, 2023 •
edited
Loading

Sorry about the delay in reviewing, it's been a busy few days. I'll make sure to review this in the next day or two :)

Thanks for your patience and for updating the title!

parlough
parlough reviewed Jul 17, 2023
View reviewed changes
Copy link
Member
@parlough parlough left a comment

Choose a reason for hiding this comment

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

Sorry again for the delay on reviewing. Thank you so much for working on this! These migrations look great plus the code seems easier to read to me.

I have a few small comments: two fixes and the rest are mostly minor formatting/readability improvements to consider.

src/assets/js/archive.js Outdated Show resolved Hide resolved
src/assets/js/archive.js Outdated Show resolved Hide resolved
src/assets/js/archive.js Outdated Show resolved Hide resolved
src/assets/js/archive.js Outdated Show resolved Hide resolved
src/assets/js/archive.js Outdated Show resolved Hide resolved
src/assets/js/archive.js Show resolved Hide resolved
src/assets/js/archive.js Outdated Show resolved Hide resolved
src/assets/js/archive.js Outdated Show resolved Hide resolved
src/assets/js/archive.js Outdated Show resolved Hide resolved
src/assets/js/archive.js Outdated Show resolved Hide resolved
@parlough parlough assigned ulisseshen and unassigned parlough Jul 17, 2023
@parlough parlough added review.await-update Awaiting Updates after Edits and removed review.tech Awaiting Technical Review labels Jul 17, 2023
ulisseshen and others added 4 commits July 17, 2023 11:02
@ulisseshen @parlough
change type fetch option param name to method
626e550 
Co-authored-by: Parker Lougheed <parlough@gmail.com>
@ulisseshen
rename variable 'aElement' to 'provenanceAnchor'
71881ba
@ulisseshen @parlough
add space to solve spacing issue
6084ed6 
Co-authored-by: Parker Lougheed <parlough@gmail.com>
@ulisseshen @parlough
Added extra line to separate code blocks better
c419f28 
Co-authored-by: Parker Lougheed <parlough@gmail.com>
ulisseshen and others added 10 commits July 17, 2023 11:27
@ulisseshen @parlough
add space to solve spacing issue
5b15ca5 
Co-authored-by: Parker Lougheed <parlough@gmail.com>
@ulisseshen @parlough
add space to solve spacing issue
3a89d52 
Co-authored-by: Parker Lougheed <parlough@gmail.com>
@ulisseshen @parlough
Improved comment to look like sentences
82055aa 
Co-authored-by: Parker Lougheed <parlough@gmail.com>
@ulisseshen @parlough
Removed console debbuging
74035bd 
Co-authored-by: Parker Lougheed <parlough@gmail.com>
@ulisseshen @parlough
Solved spacing issue
d82725b 
Co-authored-by: Parker Lougheed <parlough@gmail.com>
@ulisseshen
Expanded if and catch block for better readbility
c522b0b
@ulisseshen @parlough
Solved spacing issue
22a006c 
Co-authored-by: Parker Lougheed <parlough@gmail.com>
@ulisseshen
Moved const url to closest where it is used
4f23a38
@ulisseshen
added jsDoc on new func createTableCell;
6e2adb5
@ulisseshen
Added space to inprove readbility
e32ba6d
@ulisseshen ulisseshen requested a review from parlough July 17, 2023 17:06
parlough
parlough approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member
@parlough parlough left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is great. Thanks so much for working on this and making those adjustments! I really appreciate the help :)

@parlough parlough added st.RFM Ready to merge or land and removed review.await-update Awaiting Updates after Edits labels Jul 17, 2023
@parlough parlough changed the title Begin migrating archive.js to vanilla JavaScript Begin migrating archive.js to vanilla JavaScript Jul 17, 2023
@parlough parlough merged commit 15bc68f into flutter:main Jul 17, 2023
9 checks passed
@ulisseshen ulisseshen mentioned this pull request Jul 18, 2023
Merged
3 tasks
@sfshaza2 sfshaza2 removed the st.RFM Ready to merge or land label Jul 26, 2023
parlough added a commit that referenced this pull request Jul 26, 2023
@ulisseshen @parlough
Finish migrating archive.js file to pure js (#9063)
7295dcd 
Related with #8889 and #9022 

---------

Co-authored-by: Parker Lougheed <parlough@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parlough parlough parlough approved these changes

@sfshaza2 sfshaza2 Awaiting requested review from sfshaza2 sfshaza2 is a code owner

@atsansone atsansone Awaiting requested review from atsansone

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ulisseshen @parlough @atsansone @sfshaza2