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

Standardise module #78

Closed
13 tasks done
emteknetnz opened this issue Sep 14, 2023 · 5 comments
Closed
13 tasks done

Standardise module #78

emteknetnz opened this issue Sep 14, 2023 · 5 comments

Comments

@emteknetnz
Copy link
Member

emteknetnz commented Sep 14, 2023

Rough list of things we could do to this module to get it in line with other Silverstripe modules

Note will release an new major so can make API breaks:

Note there's a second look details arou

Task list

Architecture

Code quality

  • Add strong typing
  • Look at unit tests coverage and add more if it makes sense to
  • Consider turning the two LinkField-related traits into an abstract class instead, which LinkField and MultiLinkField can be subclasses of
    • Look at other code duplication that can also be moved into that abstract class
  • Delete unused code (e.g. client/src/LinkModal/FileLinkModal.js)

Maintenance

  • Rename .github/workflows/main.yml to .github/workflows/ci.yml
  • Run module standardiser, will add things like dispatch-ci.yml and keepalive.yml
  • Update module standardiser to remove other old meta files e.g. .codeclimate.yml etc
  • Update readme badges, get them consistent with other modules
  • Fix typos e.g. LinkTypeResolver.php "provdied"
  • Follow instruction for creating new repo.
  • Add to transifex
  • Add to kitchen sink

PRs

@maxime-rainville
Copy link

maxime-rainville commented Sep 18, 2023

Just a bit of extra context.

DBLink

The DBLink implementation was something I put in there because @chillu was concerned adding tables for each link type would clog the database. I for one never was a big fan of this.

I think the fact that most people naturally went for the Link DataObject shows that most dev prefer this approach because it's closer to what they are used to.

If we ditch DBLink there's a lot of extra simplication we can put in place. The following classes can be ditch altogether:

Relationship to Any Field

With the AnyField module, I took the LinkField logic and made it more generic so it could be used to manage any DataObject.

I think there's a strong case to make that AnyField is better in every possible way than the current linkfield.

  • I can manage list of links as well as single links.
  • It has better UI (support for icons and loading animation).
  • It allows you to constrain what type of links can be created in a specific instance.
  • Once this PR is merged, it will allow you to reorder things.

I've also added pretty comprehensive PHPUnit and Behat test to the AnyField using Links as a test model.

I'm thinking, either:

  • we just tell people to use the AnyField to manage links or,
  • we implement a LinkField that simply extends the regular AnyField with a bit of extra Link specific functionality.

Migration

Given the little adoption of DBLink, I don't think there's much value in providing a migration task.

There could be a better case to provide a migration task from other module to this one.

Other concerns

@emteknetnz
Copy link
Member Author

emteknetnz commented Sep 18, 2023

I dunno about Anyfield given the context here is linkfield support. I don't doubt that the core idea of anyfield could be useful (as I understand it the core thing is that you can manage DataObject's in a modal rather than on a regular form, correct me if I'm wrong).

It's just the context here is we're looking at bumping up the support given to the linkfield module which already has a pretty reasonable amount of real world usage.

Seems that bringing in anyfield is increasing the scope to "better support managing dataobjects in a modal.". Wouldn't that sort of functionality should maybe be added directly in silverstripe/admin rather than a module called silverstripe/linkfield?

@emteknetnz
Copy link
Member Author

Some of the things on this issue where implemented here #84

@maxime-rainville
Copy link

I'll put this card up for refinement, but it seems way too big to be done in one go. So we'll probably split it up.

@GuySartorelli
Copy link
Member

Reopening because I found some more things that aren't quite standardised that I somehow missed last time.

@GuySartorelli GuySartorelli reopened this Feb 15, 2024
@GuySartorelli GuySartorelli removed their assignment Feb 15, 2024
@emteknetnz emteknetnz self-assigned this Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants