[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

Finished migrate archive.js file to pure js #9063

Merged
merged 31 commits into from
Jul 26, 2023
Merged

Finished migrate archive.js file to pure js #9063

merged 31 commits into from
Jul 26, 2023

Conversation

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

Description of what this PR is changing or adding, and why:

Related with #8889 and #9022

Presubmit checklist

@ulisseshen ulisseshen changed the title Finish migrate arvhive.js file to pure js Finished migrate archive.js file to pure js Jul 18, 2023
@parlough parlough added the review.tech Awaiting Technical Review label Jul 18, 2023
@parlough parlough self-assigned this Jul 18, 2023
@parlough
Copy link
Member

Thanks for finishing migrating this file. I'm excited to take a look!

Can you handle resolving the conflicts with the main branch?

@ulisseshen
Copy link
Contributor Author

Thanks for finishing migrating this file. I'm excited to take a look!

Can you handle resolving the conflicts with the main branch?

I will settle this now. I didn't mind when I saw this conflict notice because I'm new to contributions, and I thought it wouldn't affect.

parlough
parlough requested changes Jul 20, 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.

Thanks again for working on this! Overall really clean migrations. 🥳

When testing I found a few small issues. Let me know if I made a mistake or you want to discuss any of them further.

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 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 Show resolved Hide resolved
@ulisseshen ulisseshen requested a review from parlough July 21, 2023 13:16
parlough
parlough requested changes Jul 24, 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 making those adjustments. Mostly looks good!

I noticed one remaining issue though, and left a potential solution.

src/assets/js/archive.js Outdated Show resolved Hide resolved
@parlough parlough added review.await-update Awaiting Updates after Edits and removed review.tech Awaiting Technical Review labels Jul 24, 2023
@parlough parlough assigned ulisseshen and unassigned parlough Jul 24, 2023
parlough
parlough reviewed Jul 24, 2023
View reviewed changes
src/assets/js/archive.js Outdated Show resolved Hide resolved
@ulisseshen ulisseshen requested a review from parlough July 24, 2023 21:53
@ulisseshen
Copy link
Contributor Author

changes are made! I was unable to assing this to review.

@parlough
Copy link
Member

Thanks for the ping @ulisseshen and sorry for the delay! I plan to get to finalizing my review today :)

@parlough parlough added review.tech Awaiting Technical Review and removed review.await-update Awaiting Updates after Edits labels Jul 26, 2023
@parlough parlough assigned parlough and unassigned ulisseshen Jul 26, 2023
@parlough parlough removed the review.tech Awaiting Technical Review label Jul 26, 2023
parlough
parlough approved these changes Jul 26, 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.

Thank you so much for all of your work on this! The changes look and work great.

@parlough parlough merged commit 7295dcd into flutter:main Jul 26, 2023
9 checks passed
@ulisseshen
Copy link
Contributor Author

I feel much happy for contribute on this project, in this migrate task in particular.

I'll continue work on this. @parlough you can help me guided me in which another file I can work on.

@parlough
Copy link
Member

Glad to hear that. Thanks for your continued help!

I haven't taken a close look at the code, but consider checking out main.js and converting some of it. There might be some code that interacts with bootstrap that is a challenge to convert, so feel free to skip those if needed. Always feel free to reach out with questions too!

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

@parlough parlough

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ulisseshen @parlough