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

Remove inline js from ia_thirdparty_logins.html #8476

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Remove inline js from ia_thirdparty_logins.html #8476

merged 2 commits into from
Feb 29, 2024

Conversation

AdwaitSalankar
Copy link
Contributor

@AdwaitSalankar AdwaitSalankar commented Oct 28, 2023 •
edited
Loading

Closes #8379

Removes inline js from ia_thirdparty_logins.html and moves it to ia_thirdparty_logins.js.

Also,

  • Changed == to ===
  • Changed var to const
  • Removed unneeded \ from regex
  • Changed the string to template literal.

in the handleMessageEvent function

Stakeholders

@RayBB

@AdwaitSalankar AdwaitSalankar force-pushed the remove-inline-js-ia-thirdparty-logins branch from f62a34f to a9f5423 Compare October 28, 2023 19:22
@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 16.04%. Comparing base (f17242e) to head (5b3da60).

Files Patch % Lines
...ary/plugins/openlibrary/js/ia_thirdparty_logins.js 0.00% 6 Missing and 5 partials ⚠️
openlibrary/plugins/openlibrary/js/index.js 0.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8476      +/-   ##
==========================================
- Coverage   16.09%   16.04%   -0.06%     
==========================================
  Files          88       89       +1     
  Lines        4671     4686      +15     
  Branches      812      818       +6     
==========================================
  Hits          752      752              
- Misses       3418     3427       +9     
- Partials      501      507       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AdwaitSalankar
Copy link
Contributor Author

@RayBB Can you review this?

RayBB
RayBB requested changes Oct 29, 2023
View reviewed changes
Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

Generally looks okay just one suggestion. We'll need a review from a staff member to merge it. Maybe @jimchamp who has reviewed a few of these.

openlibrary/plugins/openlibrary/js/index.js Outdated Show resolved Hide resolved
@AdwaitSalankar AdwaitSalankar force-pushed the remove-inline-js-ia-thirdparty-logins branch from a9f5423 to c23404c Compare October 29, 2023 19:53
@AdwaitSalankar
Copy link
Contributor Author

@RayBB I have made the changes.

RayBB
RayBB approved these changes Oct 29, 2023
View reviewed changes
Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

Everything looks good to me. We'll need someone else to give a final check and merge.
@jimchamp maybe can help or tag someone who can

@RayBB RayBB added the Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] label Oct 29, 2023
@AdwaitSalankar
Copy link
Contributor Author

AdwaitSalankar commented Oct 30, 2023 •
edited
Loading

Hi @jimchamp, @mekarpeles,@jdlrobson
can you review this PR?

@mekarpeles mekarpeles added the Priority: 3 Issues that we can consider at our leisure. [managed] label Oct 30, 2023
@AdwaitSalankar
Copy link
Contributor Author

AdwaitSalankar commented Oct 31, 2023 •
edited
Loading

@cdrini
Can you review this PR?

@cdrini
Copy link
Collaborator

cdrini commented Dec 5, 2023

Hi @AdwaitSalankar ! Just a heads up , this one will take a little longer for us to get to because it touches the login pages which we can only fully test on staging/production, which requires more time from us to get through. Feel free to tackle another issue in the meantime!

@mekarpeles mekarpeles assigned jimchamp and unassigned cdrini Feb 26, 2024
@jimchamp jimchamp added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Feb 29, 2024
jimchamp
jimchamp approved these changes Feb 29, 2024
View reviewed changes
Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

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

Thanks for helping with this, @AdwaitSalankar! Sorry that it took so long to review....

Everything seems to be working as expected. Was able to link an existing archive account to an OL account and login in testing.

@jimchamp jimchamp merged commit 40b17fc into internetarchive:master Feb 29, 2024
4 checks passed
@jimchamp jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimchamp jimchamp jimchamp approved these changes

@RayBB RayBB RayBB approved these changes

Assignees

@jimchamp jimchamp

Labels
Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] Priority: 3 Issues that we can consider at our leisure. [managed]
Projects
Ray's Project
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove inline js: account/ia_thirdparty_logins.html
6 participants
@AdwaitSalankar @codecov-commenter @cdrini @RayBB @jimchamp @mekarpeles