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

DEP Update jquery-ui in thirdparty folder #1424

Merged
merged 1 commit into from
Jan 12, 2023
Merged

DEP Update jquery-ui in thirdparty folder #1424

merged 1 commit into from
Jan 12, 2023

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Dec 22, 2022 •
edited
Loading

Issue #1423

jquery-ui v1.9.2 -> v1.13.2

This should have been upgraded as part of 1.12, and since there's an existing regression I think we should do a patch release in 1.12 rather than waiting for 1.13

Updated because of this answer https://stackoverflow.com/a/37925405 - javascript error matched what console error I saw after running yarn dev and replicating issue

Issue could be replicated by

  • Creating a test member in the security section of the cms
  • Editing a group in the group section of the security section and attempting to add the new member to the group using the autocompleter

@emteknetnz emteknetnz mentioned this pull request Dec 22, 2022
Closed
@emteknetnz emteknetnz force-pushed the pulls/1.12/jquery-ui-upgrade branch from 4f6b08d to 5601812 Compare December 22, 2022 02:28
@emteknetnz emteknetnz marked this pull request as draft December 22, 2022 02:29
@emteknetnz emteknetnz force-pushed the pulls/1.12/jquery-ui-upgrade branch 2 times, most recently from 7388260 to 6da56a8 Compare December 22, 2022 02:56
@kinglozzer kinglozzer mentioned this pull request Jan 3, 2023
Closed
@emteknetnz emteknetnz force-pushed the pulls/1.12/jquery-ui-upgrade branch 3 times, most recently from 1d75b9b to 6529629 Compare January 9, 2023 03:30
emteknetnz
emteknetnz commented Jan 9, 2023
View reviewed changes
client/src/legacy/LeftAndMain.js

// Allow opt-out via data element or entwine property.
if($(el).data('ignoreTabState') || $(el).getIgnoreTabState()) return;

selectedTabs.push({id:id, selected:$(el).tabs('option', 'selected')});
selectedTabs.push({id:id, selected:$(el).tabs('option', 'active')});
Copy link
Member Author

Choose a reason for hiding this comment

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

'selected' was renamed to 'active' - https://bugs.jqueryui.com/ticket/7135

emteknetnz
emteknetnz commented Jan 9, 2023
View reviewed changes
client/src/legacy/LeftAndMain.js
@@ -889,12 +889,12 @@ $.entwine('ss', function($) {
this.find('.cms-tabset,.ss-tabset').each(function(i, el) {
var id = $(el).attr('id');
if(!id) return; // we need a unique reference
if(!$(el).data('tabs')) return; // don't act on uninit'ed controls
if(!$(el).data('uiTabs')) return; // don't act on uninit'ed controls
Copy link
Member Author

Choose a reason for hiding this comment

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

'tabs' was renamed to 'uiTabs' in jquery-ui 1.10 - I couldn't find a reference of where this was done, though console.log'ing $el.data() showed it to now be 'uiTabs'

emteknetnz
emteknetnz commented Jan 9, 2023
View reviewed changes
client/src/styles/legacy/_uitheme.scss
color: $body-color;
font-size: 1em;
border: 0;

a {
color: $link-color;
Copy link
Member Author

Choose a reason for hiding this comment

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

These CSS changes are effectively to revert some CSS additions that went into later versions of jquery-ui

@emteknetnz emteknetnz marked this pull request as ready for review January 9, 2023 03:33
@emteknetnz emteknetnz force-pushed the pulls/1.12/jquery-ui-upgrade branch 2 times, most recently from 25ee1c3 to a7af0d3 Compare January 9, 2023 08:01
@GuySartorelli
Copy link
Member

@emteknetnz In the installer CI run you linked to, everything is failing... does that link need to be updated to a more recent run? Or does this PR legitimately break everything?

GuySartorelli
GuySartorelli requested changes Jan 10, 2023
View reviewed changes
Copy link
Member

@GuySartorelli GuySartorelli 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.

The cursor should be pointer when hovering over tabs.

The icon alignment (super minor so not too fussed) and text colour of expandable composite fields is not correct.
before:
image
this PR:
image

The arrow icon on expandable composite fields disappears when expanding the field - it should not.

Everything else looks fine - and the reported regression is resolved.

@emteknetnz emteknetnz force-pushed the pulls/1.12/jquery-ui-upgrade branch from a7af0d3 to 3adf74f Compare January 10, 2023 00:49
@emteknetnz
Copy link
Member Author

@GuySartorelli updated

@GuySartorelli
Copy link
Member

GuySartorelli commented Jan 10, 2023 •
edited
Loading

@emteknetnz Can you please respond to my question about the installer CI run?
You also only seem to have addressed one of the three regressions I mentioned.

@emteknetnz emteknetnz force-pushed the pulls/1.12/jquery-ui-upgrade branch from 3adf74f to 05f7103 Compare January 11, 2023 21:50
@emteknetnz emteknetnz force-pushed the pulls/1.12/jquery-ui-upgrade branch from 05f7103 to 50e343a Compare January 11, 2023 21:53
GuySartorelli
GuySartorelli approved these changes Jan 12, 2023
View reviewed changes
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Looks good, works great locally.

@GuySartorelli GuySartorelli merged commit 5497d5b into silverstripe:1.12 Jan 12, 2023
@GuySartorelli GuySartorelli deleted the pulls/1.12/jquery-ui-upgrade branch January 12, 2023 04:08
@GuySartorelli
Copy link
Member

Released as 1.12.1

@sabina-talipova sabina-talipova mentioned this pull request Mar 27, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuySartorelli GuySartorelli GuySartorelli approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emteknetnz @GuySartorelli