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

ISSUE-167: Style "share" pop-up window for IA Bookreader #264

Merged
merged 1 commit into from
Feb 2, 2023
Merged

ISSUE-167: Style "share" pop-up window for IA Bookreader #264

merged 1 commit into from
Feb 2, 2023

Conversation

aksm
Copy link
Contributor

@aksm aksm commented Feb 1, 2023

Resolves #167.

Phew, that was an unexpected journey. The Javascript and CSS are not ideal, but between clashing library themes, I did the best I could do. Needs some testing, particularly regarding the page/book URLs being generated and the various platform share URLs being generated/updated.

…ke everything funtion and look reasonable.
@aksm aksm self-assigned this Feb 1, 2023
@aksm
Copy link
Contributor Author

aksm commented Feb 1, 2023

@DiegoPino will do some cross-browser testing on an archipelago-deployment-live (Facebook sharing doesn't work on localhost, though just switching to a public URL does) just to be sure, but glad to take any feedback at this stage if anything looks obviously problematic.

@aksm aksm marked this pull request as ready for review February 2, 2023 15:44
@aksm
Copy link
Contributor Author

aksm commented Feb 2, 2023

Tested on latest Chrome, Safari, and Edge browsers on Linux, Mac, and Windows.

@aksm aksm requested a review from DiegoPino February 2, 2023 15:45
Copy link
Member

@DiegoPino DiegoPino left a comment

Choose a reason for hiding this comment

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

Good! thanks!

shareBody.append(shareCheck, shareButtonContainer);
e.append(shareTitle, shareBody);
};

Copy link
Member

Choose a reason for hiding this comment

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

wow this is a lot of code and JS ... do you think we could move a few of the "var" to "const" to help with memory here?
I'm trying to figure out performance lately and seems like our amount of JS is a bit over the roof. Just an idea. Need to test this before merging! Thanks a lot for your work on this!

@DiegoPino DiegoPino merged commit 3941cce into esmero:1.1.0 Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants