-
Notifications
You must be signed in to change notification settings - Fork 35
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
FIX Ensure correct link is used when gridfield is readonly #90
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to create a pull request for this issue! I haven't had time yet to test the code out, but thought I'd mention that I'm not super happy about the approach.
I don't think that overloading the GridField class is necessary to fix your issue - I also think you should be able to use GridFieldConfig to target and replace the View button with what appears to be a sort of Edit button. If you can do this (I haven't looked far enough into it yet) then you can get away with less changes and less new APIs introduced.
I'll pick this up later this week and have a closer look. In the meantime, please let me know what you think =)
I agree with @robbieaverill - only a single class needs to be introduced (the new |
The thing is, there is no View button in GridField config, so I can not replace it. There is Edit button that gets replaced later when canEdit check is made LeftAndMain::getEditForm (Edit button is removed somewhere after this)
and in the end View button is added in GridField::performReadonlyTransformation
I didn't find any hook after that to replace View button with my extended View button, that's why I had to introduce an extended GridField and overload performReadonlyTransformation function. I hope you find a better solution. |
I'll take a look at this later this week :) |
639128b
to
e2e7ec1
Compare
e2e7ec1
to
e9956f4
Compare
e9956f4
to
126a7df
Compare
I've updated this PR to use the newish API for updating the set of readonly components. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, works well
Requires silverstripe/silverstripe-framework#11228
Issue