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

Add README documentation to ExtensionPage #3094

Merged
merged 26 commits into from
Oct 28, 2021
Merged

Conversation

imorland
Copy link
Member

@imorland imorland commented Oct 6, 2021

Changes proposed in this pull request:
Adds a button to display an extensions README file within a modal directly from the ExtensionPage

Reviewers should focus on:
This is almost a "lift and shift" from blomstra/readme. Did I forget any core conventions when migrating this?

Screenshot
image
image

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Copy link
Member

@davwheat davwheat left a comment

Choose a reason for hiding this comment

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

All looks good on the whole, just a few nitpicks.

locale/core.yml Show resolved Hide resolved
js/src/admin/components/ExtensionPage.js Show resolved Hide resolved
js/src/admin/components/ExtensionPage.js Show resolved Hide resolved
js/src/admin/components/ReadmeModal.js Outdated Show resolved Hide resolved
@imorland imorland requested a review from davwheat October 6, 2021 11:05
Copy link
Member

@luceos luceos left a comment

Choose a reason for hiding this comment

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

Approving based on my experiences inside our hosting stack.

Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

I like the frontend a lot, and think this would be a great feature. From a code organization perspective, I'm not sure that the controller directly is the best place for this implementation to live. I'm also hesitant to add new endpoints that are far from complying with the JSON:API spec if not necessary. Left some ideas in the code.

src/Api/Controller/ReadmeController.php Outdated Show resolved Hide resolved
src/Api/routes.php Outdated Show resolved Hide resolved
@imorland
Copy link
Member Author

So @askvortsov1 and I discussed this again today. We decided to simplify things a little and not go down the whole boilerplate building excercise just yet.

Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Overall looks good, the one thought I have is possibly changing extensions-readme to extension-readmes to show that it's plural in the readmes, not the extensions.

js/src/admin/components/ReadmeModal.js Outdated Show resolved Hide resolved
locale/core.yml Outdated Show resolved Hide resolved
Co-authored-by: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com>
@askvortsov1
Copy link
Sponsor Member

Thank you so much for this!

@askvortsov1 askvortsov1 merged commit 28ead83 into flarum:master Oct 28, 2021
@askvortsov1 askvortsov1 added this to the 1.2 milestone Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants