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

NEW Extend new AbstractGridFieldComponent class #134

Merged

Conversation

GuySartorelli
Copy link
Contributor

@GuySartorelli GuySartorelli commented Feb 12, 2022

This makes the GridFieldSortableRows component Injectable, and allows any future enhancements in the new abstract class to automatically apply without requiring additional changes in this module.

The class is introduced in silverstripe/framework 4.11.0 (see silverstripe/silverstripe-framework#10204) so the dependency constraint needs to be updated.

Some whitespace issues have also been fixed automatically by my IDE.

This makes the `GridFieldSortableRows` component `Injectable`, and
allows any future enhancements in the new abstract class to
automatically apply without requiring additional changes in this module.

The class is introduced in silverstripe/framework 4.11.0 so the
dependency constraint needs to be updated.
@GuySartorelli
Copy link
Contributor Author

GuySartorelli commented Feb 12, 2022

Note that tests will error until a 4.11 branch or tag is available in silverstripe/silverstripe-framework

@UndefinedOffset
Copy link
Owner

UndefinedOffset commented Feb 16, 2022

Thanks @GuySartorelli I'll merge this after I do #135, the tests ironically are broken at the moment anyways because this repo was using travis-ci.org which of course is now defunct. I've been using GitHub Actions quite a bit for work with @webbuilders-group and it's been quite good so I'm probably going to switch the repo over to that first before I merge this.

@UndefinedOffset UndefinedOffset linked an issue Feb 16, 2022 that may be closed by this pull request
@UndefinedOffset
Copy link
Owner

Ok tests are now running on GitHub Actions, I'm going to hold on this for now until the 4.11 release is out then I'll merge this and tag likely as 3.0.0 (adjusting builds accordingly) as this is a breaking change.

@GuySartorelli
Copy link
Contributor Author

GuySartorelli commented Feb 16, 2022

Makes sense! Not sure how it breaks BC (unless someone has code that relies on GridFieldSortableRows not being injectable or not having a superclass?) but any release is better than no release. Thanks for letting me know :)

@emteknetnz
Copy link
Contributor

@UndefinedOffset this isn't a breaking change, you shouldn't need to release a new major

Though yes, you will need to hold off until framework 4.11.0 has been released

@UndefinedOffset UndefinedOffset merged commit f5a5346 into UndefinedOffset:master Jul 6, 2022
@UndefinedOffset
Copy link
Owner

UndefinedOffset commented Jul 6, 2022

Tagged as 2.1.0, thanks!

@GuySartorelli GuySartorelli deleted the enh/injectable branch July 6, 2022 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move to tests to GitHub Actions
3 participants