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

Small prod updates #44

Merged
merged 2 commits into from
Jun 8, 2021
Merged

Small prod updates #44

merged 2 commits into from
Jun 8, 2021

Conversation

frankduncan
Copy link
Collaborator

What it says on the tin :)

Frank Duncan added 2 commits June 8, 2021 13:30
SimpleBook doesn't really support getting a physical copy from
PediaPress anymore, like Collection used to.  So the text was
inaccurate.
This was preventing images from loading up when Collection wasn't also
installed.
@frankduncan frankduncan requested a review from slifty June 8, 2021 18:33
Copy link
Member

@kfogel kfogel left a comment

Choose a reason for hiding this comment

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

The fact that this is in a file named CollectionAjaxFunctions.php makes me feel weird about just changing the path component from "Collections" to "SimpleBook". I mean, if this is just a quick change to get something that can be rolled out to production right away, then I get it, but let's file a ticket about doing a more thorough adjustment later.

(Hunh. I made this comment directly on the particular commit that was about the image path being hardcoded, but somehow my comment ended up being associated with the overall PR? Thanks, GitHub?)

@slifty
Copy link
Contributor

slifty commented Jun 8, 2021

@kfogel agreed in essence. There is already an issue about moving away from Collection (#25) and I think your concern applies to that / I've now linked your comment to that issue.

There is a lot of tech debt in this plugin right now, and it needs some focused gutting.

We shouldn't rename files right now, as that is part of the larger required refactor and I think that a half step is more likely to cause trouble and confusion in the short term.

Copy link
Contributor

@slifty slifty left a comment

Choose a reason for hiding this comment

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

Worth noting here as well that we all know this is a patch and that ideally these assets would not be hardcoded with the plugin name. It's not obvious the "right" way to do that, so this is a fine solution in the mean time!

@frankduncan frankduncan merged commit c0fb30a into main Jun 8, 2021
@frankduncan frankduncan deleted the small-prod-updates branch June 8, 2021 18:46
@frankduncan frankduncan restored the small-prod-updates branch June 8, 2021 18:47
@frankduncan frankduncan deleted the small-prod-updates branch June 8, 2021 18:47
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