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

wip: silverstripe 5 compatibility #777

Closed
wants to merge 16 commits into from

Conversation

tractorcow
Copy link
Collaborator

@tractorcow tractorcow commented Feb 7, 2023

This is just a preview PR to be replaced once the current WIP is ready to merge.

// force ... to turn into actual title
// @todo i18n this
content: "Copy from" !important;
content: "Copy from" !important;// sass-lint:disable-line no-important
Copy link
Collaborator Author

@tractorcow tractorcow Feb 7, 2023

Choose a reason for hiding this comment

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

This was a really ugly work-around because I couldn't update the button labels in react grid menu.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth adding that as a comment above it - it's always nice when looking at scss which has exceptions to see comments right there telling you why it's an exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it was '...' which is the default label for 'more actions'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh that makes sense haha ignore me then, I just didn't understand that comment that was already there 😅

@@ -1,5 +1,5 @@
{
"name": "tractorcow/silverstripe-fluent",
"name": "silverstripe/silverstripe-fluent",

Choose a reason for hiding this comment

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

That was only added here so our fork could be installed under its own name.

Suggested change
"name": "silverstripe/silverstripe-fluent",
"name": "tractorcow/silverstripe-fluent",

Comment on lines +1 to +3
# WARNING

This is a temporary fork of `tractorcow/silverstripe-fluent`. Do not install in production environment.

Choose a reason for hiding this comment

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

Suggested change
# WARNING
This is a temporary fork of `tractorcow/silverstripe-fluent`. Do not install in production environment.

Comment on lines +37 to +38
Requirements::javascript('silverstripe/silverstripe-fluent:client/dist/js/fluent.js');
Requirements::css("silverstripe/silverstripe-fluent:client/dist/styles/fluent.css");

Choose a reason for hiding this comment

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

Suggested change
Requirements::javascript('silverstripe/silverstripe-fluent:client/dist/js/fluent.js');
Requirements::css("silverstripe/silverstripe-fluent:client/dist/styles/fluent.css");
Requirements::javascript('tractorcow/silverstripe-fluent:client/dist/js/fluent.js');
Requirements::css("tractorcow/silverstripe-fluent:client/dist/styles/fluent.css");

Comment on lines +26 to +27
Requirements::javascript("silverstripe/silverstripe-fluent:client/dist/js/fluent.js");
Requirements::css("silverstripe/silverstripe-fluent:client/dist/styles/fluent.css");

Choose a reason for hiding this comment

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

Suggested change
Requirements::javascript("silverstripe/silverstripe-fluent:client/dist/js/fluent.js");
Requirements::css("silverstripe/silverstripe-fluent:client/dist/styles/fluent.css");
Requirements::javascript("tractorcow/silverstripe-fluent:client/dist/js/fluent.js");
Requirements::css("tractorcow/silverstripe-fluent:client/dist/styles/fluent.css");

@GuySartorelli
Copy link
Contributor

A lot of these commits are already in this repo - they should be merged up to master before this pr gets merged.

@tractorcow
Copy link
Collaborator Author

Do you want me to change the source from silverstripe:7 to silverstripe:master

@GuySartorelli
Copy link
Contributor

GuySartorelli commented Feb 12, 2023

No, silverstripe:7 is the correct source. It just has some commits from this repo which aren't in this repo's master branch (I'm guessing when the branch was made it was branched off 6 instead of master. The 6 branch in this repo has some commits that are currently missing from master, so it needs to be merged up)

@GuySartorelli
Copy link
Contributor

GuySartorelli commented Feb 15, 2023

@tractorcow if you can merge up the 6 branch to master (see master...6) then I'll update this PR with the recommended changes and we'll be able to get it merged.

Ideally we would also have a 7 branch instead of master but that's neither here nor there.

@tractorcow
Copy link
Collaborator Author

I'll do this soon. I will make a 7 branch once we tag our first 7.0.0

@tractorcow
Copy link
Collaborator Author

tractorcow commented Feb 16, 2023

Completed.

c042104

@GuySartorelli
Copy link
Contributor

GuySartorelli commented Feb 16, 2023

Sweet, thank you. I'll update the PR in a moment

@GuySartorelli
Copy link
Contributor

I've opted to open a new PR, since I can't amend this one and didn't want to affect our current CI by making changes to the fork used in the beta
#779

@tractorcow
Copy link
Collaborator Author

Replaced with #779

@tractorcow tractorcow closed this Feb 20, 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.

5 participants