Skip to content

Conversation

@thapacodes4u
Copy link
Contributor

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Copy link
Member

@SlimDeluxe SlimDeluxe left a comment

Choose a reason for hiding this comment

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

  1. Please separate the CSS and JS into their own asset files and register them with Filament.
    https://filamentphp.com/docs/3.x/support/assets
  2. Instead of using a macro, create a new column class that extends the ImageColumn class (use the same name for the class, just the namespace is different). Place it in src/Filament/Tables/Columns. "Preview" can then be a method with proper parameters that will get hinted when writing the column definition.

Copy link
Member

@SlimDeluxe SlimDeluxe left a comment

Choose a reason for hiding this comment

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

Please see below for required changes

@SlimDeluxe
Copy link
Member

After the changes, a fix will be required in DataLinx/eclipsephp-catalogue-plugin#28

@thapacodes4u
Copy link
Contributor Author

@SlimDeluxe Should be good to review this

@thapacodes4u thapacodes4u deleted the refactor/image-lightbox branch September 26, 2025 07:54
@thapacodes4u thapacodes4u restored the refactor/image-lightbox branch September 26, 2025 07:54
@thapacodes4u thapacodes4u reopened this Sep 30, 2025
@thapacodes4u
Copy link
Contributor Author

@SlimDeluxe This got closed by mistake. And I think we should wait until we migrate the project to F4 before we merge this.

@thapacodes4u
Copy link
Contributor Author

@SlimDeluxe So I am converting this to draft.

@thapacodes4u thapacodes4u marked this pull request as draft September 30, 2025 10:59
@thapacodes4u thapacodes4u marked this pull request as ready for review October 5, 2025 14:51
@thapacodes4u
Copy link
Contributor Author

@SlimDeluxe since we won't be merging F4 anytime soon so maybe this can be reviewed & merged.

@SlimDeluxe SlimDeluxe merged commit 330c775 into DataLinx:main Oct 6, 2025
12 checks passed
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