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

Remove jQuery from user notes preview #529

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

cbravobernal
Copy link
Contributor

@cbravobernal cbravobernal commented Jul 22, 2024

What

Remove jQuery from user-notes-preview.js

Why

jQuery is known to inflate the bundle size of packages, and in most cases, it can be replaced with native language functionality.

@cbravobernal cbravobernal marked this pull request as ready for review July 23, 2024 08:46
@cbravobernal cbravobernal changed the title Draft: Remove jQuery from user notes preview Remove jQuery from user notes preview Jul 23, 2024
} );
// Make first child of the preview focusable.
if ( preview.firstChild ) {
preview.firstChild.setAttribute( 'tabindex', '0' );
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting an error when I test this, it looks like firstChild is a text node, not an html element.

Screenshot 2024-07-25 at 3 52 51 PM

Copy link
Member

Choose a reason for hiding this comment

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

👍 Looks like a place for firstElementChild

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 6762ec8

It's weird I didn't get the error until now 🤔

Comment on lines +17 to +19
spinner = document.createElement( 'span' );
spinner.className = 'spinner';
spinner.style.display = 'none';
Copy link
Contributor

Choose a reason for hiding this comment

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

This does create the spinner correctly, but apparently it was never styled when we moved to the block theme, so nothing is visible. Not a problem for this PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where can I check the old styles?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't copy over the old styles, but you could borrow the loading animation from this CSS (it's the loading state for images such as the pattern & style variation screenshots on Themes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spinner didn't have any styled, an empty span was appearing. I added the style, but the time of the spinner is quite small.

Screen.Recording.2024-07-29.at.16.46.45.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review (PRs only)
Development

Successfully merging this pull request may close these issues.

3 participants