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

FIX Correctly close spans for btn loading icon #1431

Merged

Conversation

GuySartorelli
Copy link
Member

The self-closing span tags were being incorrectly interpreted as nested spans, instead of sibling spans. This is most likely a result of the recent update of jQuery.

The correct HTML syntax for these is to not have them self-closing.

Parent issue

Copy link
Contributor

@michalkleiner michalkleiner left a comment

Choose a reason for hiding this comment

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

Looks ok, merge when you're happy with the tests, not sure if the failures are related or not. The change is fine on its own.

@GuySartorelli GuySartorelli force-pushed the pulls/1.12/loading-icon branch from 6de86e9 to df8e1c1 Compare January 12, 2023 04:44
@GuySartorelli
Copy link
Member Author

Rebased and rebuilt js - the actual change is unaltered.

@kinglozzer
Copy link
Member

Running git diff
git diff found modified files when it should not have:
M js/bundle.js

Do we block merge for that? If not, feel free to self merge @GuySartorelli

@GuySartorelli
Copy link
Member Author

We do, yeah. It means that when CI runs yarn build it's not getting the same results I got - in a community PR I'd be concerned they're trying to add something sneaky in there. In this PR I'm not sure what's causing it, but it's something that definitely needs resolving before we can merge.

@maxime-rainville
Copy link
Contributor

maxime-rainville commented Feb 12, 2023

@kinglozzer An attacker could try to build a bundle that doesn't match the PR and sneak a backdoor in the bundle.

@maxime-rainville
Copy link
Contributor

I'm 90% sure the failure is a glitch. I forced another build, which will hopefully fix this.

@kinglozzer
Copy link
Member

@GuySartorelli could you try rebuilding the bundle locally? CI checks are still failing 😞

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Feb 13, 2023

I have tried that already (I think? I can't see it in the PR history though..... maybe my memory is playing tricks) but will try again. We've had this problem once before with one of Steve's PRs, and when I created an identical PR to try to debug it my PR didn't have this problem.... so if it doesn't work this time I'll try opening a new PR (in draft initially just to see if it works) - and failing that I guess someone can build it locally and do a diff to validate that they get the same results I push up?
We'll see.

@GuySartorelli GuySartorelli force-pushed the pulls/1.12/loading-icon branch from df8e1c1 to 7aa0d88 Compare February 13, 2023 20:33
@GuySartorelli
Copy link
Member Author

There was a change when I yarn builded for some reason... not sure why but hopefully that fixes it.
Hurray for CI catching things yarn itself didn't.

@GuySartorelli
Copy link
Member Author

This is finally green and already has two approvals. Self-merging.

@GuySartorelli GuySartorelli merged commit 99082e8 into silverstripe:1.12 Feb 14, 2023
@GuySartorelli GuySartorelli deleted the pulls/1.12/loading-icon branch February 14, 2023 02:36
@GuySartorelli
Copy link
Member Author

@lekoala Released as 1.12.2

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.

4 participants