Skip to content

Conversation

@ioigoume
Copy link
Contributor

@ioigoume ioigoume commented Nov 19, 2024

Closes #26

@ioigoume ioigoume marked this pull request as draft November 19, 2024 18:08
@tvdijen tvdijen force-pushed the SSP-2066_add_routes_and_controllers branch from d8d4fc7 to f5170a1 Compare November 20, 2024 15:50
@tvdijen tvdijen force-pushed the SSP-2066_add_routes_and_controllers branch from f5170a1 to 5b93932 Compare November 20, 2024 15:52
@codecov
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 60 lines in your changes missing coverage. Please review.

Project coverage is 67.36%. Comparing base (2b946a9) to head (0961460).
Report is 83 commits behind head on master.

Additional details and impacted files
@@              Coverage Diff              @@
##             master      #45       +/-   ##
=============================================
+ Coverage     41.43%   67.36%   +25.92%     
- Complexity      204      342      +138     
=============================================
  Files            14       26       +12     
  Lines           654     1238      +584     
=============================================
+ Hits            271      834      +563     
- Misses          383      404       +21     

@ioigoume ioigoume marked this pull request as ready for review December 2, 2024 17:05
@tvdijen tvdijen mentioned this pull request Jan 8, 2025
@ioigoume ioigoume requested a review from pradtke January 9, 2025 15:54
Copy link
Collaborator

@pradtke pradtke 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 all the improvements @ioigoume . I think the last issue I ran into was with debugMode not working (and when I made some minor changes to fix it, then how the content is displayed isn't quite right)

@ioigoume ioigoume force-pushed the SSP-2066_add_routes_and_controllers branch from b2f7309 to 0bf5245 Compare January 23, 2025 10:39
@tvdijen
Copy link
Member

tvdijen commented Jan 23, 2025

I have one more remark on the public-folder.. I see we're copying in jQuery and highlight.js.

  • We have https://github.com/simplesamlphp/simplesamlphp-assets-jquery available. It can be installed as a composer-dependency and it is automatically updated when a new jQuery release is released. Files go to public/assets/jquery/ in the SimpleSAMLphp base directory.

  • highlight.js is already available in SimpleSAMLphp, so we shouldn't be needing that in this module.

Maybe we can reduce complexity of this module by re-using what's already available?

@ioigoume ioigoume requested review from pradtke and tvdijen January 23, 2025 16:56
@ioigoume
Copy link
Contributor Author

I have one more remark on the public-folder.. I see we're copying in jQuery and highlight.js.

* We have `https://github.com/simplesamlphp/simplesamlphp-assets-jquery` available. It can be installed as a composer-dependency and it is automatically updated when a new jQuery release is released. Files go to `public/assets/jquery/` _in the SimpleSAMLphp base directory_.

* highlight.js is already available in SimpleSAMLphp, so we shouldn't be needing that in this module.

Maybe we can reduce complexity of this module by re-using what's already available?

@tvdijen thank you for the hints. I refactored the code according to your recommendations.

@tvdijen
Copy link
Member

tvdijen commented Jan 23, 2025

@tvdijen thank you for the hints. I refactored the code according to your recommendations.

You have to add simplesamlphp/simplesamlphp-assets-jquery to the composer.json to be able to use it.

@ioigoume
Copy link
Contributor Author

@tvdijen thank you for the hints. I refactored the code according to your recommendations.

You have to add simplesamlphp/simplesamlphp-assets-jquery to the composer.json to be able to use it.

@tvdijen after the latest updates it is not required anymore.

Copy link
Collaborator

@pradtke pradtke left a comment

Choose a reason for hiding this comment

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

I've tested debug mode and it both works and looks much nicer than the old version.

Thanks for all your work improving this module. I think it is ready to merge and for others to be able to test. I assume a v7.0.0-rc3 release/tag after the merge is appropriate?

@tvdijen tvdijen merged commit 5b35ab7 into simplesamlphp:master Jan 24, 2025
15 checks passed
@tvdijen
Copy link
Member

tvdijen commented Jan 24, 2025

tagged v7.0.0-rc3

@ioigoume ioigoume deleted the SSP-2066_add_routes_and_controllers branch January 24, 2025 17:24
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.

Introduce Controller-classes to replace old www-dir

3 participants