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

v2: Update namespace, dependencies. Add PhoneLink & migration task #54

Merged
merged 18 commits into from
Mar 2, 2023

Conversation

chrispenny
Copy link
Collaborator

@chrispenny chrispenny commented Feb 27, 2023

Updating master (GraphQL 4) with important changes in 1 (GraphQL 3)

I think for a long time development was only being undertaken for the 1 branch, so master (which was effectively 2) has fallen quite far behind.

  • Update Github Actions and Recipe Testing
  • Update namespace from Link to LinkField.
  • Bring over the PhoneLink model.
  • Bring over the LinkableMigrationTask.
  • Add missing table_name configuration.
  • SiteTreeLink should grab Title from its page, if no title is specified.
  • Linting

What I didn't bring over

  • Client file changes (Mojmir indicated that these were just linting changes, I think bringing these over will make Add multi link support #45 more difficult to rebase).

Testing

  • Page has_one Link::class: tested within the page edit form.
  • Page has_many LinkItem, which has_one Link: tested within a GridField edit form.

Issues with GridField edit form

Currently the scaffolded field seems to be a dropdown of existing Link models, rather than a LinkField. I'm not sure why that is.

Screen Shot 2023-02-28 at 9 23 33 AM

I think this issue exists in the GraphQL 3 version as well, so probably doesn't have to be solved here.

The workaround is to remove the scaffolded field, and explicitly add a LinkField.

@chrispenny chrispenny changed the title Update dependencies. Move to Github Actions Update namespace & dependencies. Move to Github Actions Feb 27, 2023
@chrispenny chrispenny force-pushed the mnt/dependencies-github-actions branch from 173e961 to f1c7ba8 Compare February 27, 2023 19:08
@chrispenny chrispenny changed the title Update namespace & dependencies. Move to Github Actions Update namespace, dependencies. Add PhoneLink & migration task Feb 27, 2023
@chrispenny chrispenny changed the title Update namespace, dependencies. Add PhoneLink & migration task v2: Update namespace, dependencies. Add PhoneLink & migration task Feb 27, 2023
@chrispenny chrispenny marked this pull request as ready for review February 27, 2023 21:47
Copy link
Collaborator

@mfendeksilverstripe mfendeksilverstripe left a comment

Choose a reason for hiding this comment

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

Looking good overall 👍 , found a couple of minor issues though.

.github/workflows/main.yml Outdated Show resolved Hide resolved
phpunit.xml.dist Show resolved Hide resolved
Copy link

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Did a quick code review and didn't see anything obviously wrong. I'm assuming that all of this code was more or less lifted from the v1 version of the module.

I tested that the core functionality of the module still works. I tested by adding a link to a page and by creating a Link block.

Didn't test the migration ... I don't have much context on how that other module work.

My main concern is what will happen with the data with the new table name?

src/Models/Link.php Show resolved Hide resolved
@chrispenny chrispenny force-pushed the mnt/dependencies-github-actions branch from 0ab3a11 to 71c5053 Compare March 1, 2023 18:55
Copy link
Collaborator

@mfendeksilverstripe mfendeksilverstripe left a comment

Choose a reason for hiding this comment

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

Thanks for the changes 👍 I have one minor concern with the frontend assets linting but other than that it's looking really good.

.eslintignore Outdated Show resolved Hide resolved
Copy link
Collaborator

@mfendeksilverstripe mfendeksilverstripe left a comment

Choose a reason for hiding this comment

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

Awesome work 👍 I'm happy with the changes.

Copy link

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Me happy!

@chrispenny chrispenny merged commit 826eb4e into master Mar 2, 2023
@chrispenny chrispenny deleted the mnt/dependencies-github-actions branch March 2, 2023 18:03
@chrispenny chrispenny mentioned this pull request Mar 2, 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.

3 participants