Tag server-side response should extend core's Index

[askvortsov1]

Currently, there's a lot of duplication between the tags Tag content handler and core's Index content handler. Similar duplication exists in "index" blade template vs "tag" blade template. I suspect that this issue similarly affects subscribe with the following page.

We should:

1. Refactor Index so generation of params (and maybe the preloaded API doc) is split into a protected method. Also make private methods protected.
2. Inherit Index for the tag and subscription content handlers
3. Make the discussion list portion of the index blade template a reusable template that can be used in core, tags, and subscriptions

[Adams-Ijachi]

hello , i'm kinda new to this open source stuff and would love to try this out it's still open.. If i understand you correctly , i'm supposed to inherit Index for Tag and use the index's get ApiDocument as the response in Tag?

Sorry if this seems like a noob question? would and appreciate the clarification

[askvortsov1]

hello , i'm kinda new to this open source stuff and would love to try this out it's still open.. If i understand you correctly , i'm supposed to inherit Index for Tag and use the index's get ApiDocument as the response in Tag?

Sorry if this seems like a noob question? would and appreciate the clarification

Hi, welcome to Flarum! Essentially, we'll want to inherit Index for the tag and subscription "list discussions" handler, yeah. However, as part of this, it will probably be necessary to refactor the Index class a bit and extract some logic from __invoke into protected methods.

As an additional step, common code from the blade PHP template for the discussion list (flarum.forum::frontend.content.index) could be extracted into separate component blade templates and reused in extensions.

Please don't hesitate to ask here, on Discuss, or in our discord if you have any questions! Links are in the footer of https://flarum.org

[Adams-Ijachi]

ok thanks for replying , i'll start with the refactoring, is there any action i could take to test this when i make a change

[askvortsov1]

ok thanks for replying , i'll start with the refactoring, is there any action i could take to test this when i make a change

You'll want to set up a local dev Flarum instance so you can test things as you go, see https://docs.flarum.org/contributing

You can also add integration tests that check the generated HTML to make sure everything that should be present is there. https://docs.flarum.org/extend/testing has a bit more information on this.

[Adams-Ijachi]

Alright thanks kinda stuck on the serving my local installation part , trying to use xamp What i've done

• Moved Project to htdocs of xamp
• ran started and apache server
• tried accessing localhost/flarum/public

Error !!

Warning: require(C:\xampp\htdocs\flarum\vendor\composer/../flarum/core/src/helpers.php): Failed to open stream: No such file or directory in C:\xampp\htdocs\flarum\vendor\composer\autoload_real.php on line 71

and i have done composer install and update (as potential fixes and not really helped) , also asked the same question on the discord group

SOLVED had to delete vendor folder and then run composer install again.

[askvortsov1]

Alright thanks kinda stuck on the serving my local installation part , trying to use xamp What i've done

• Moved Project to htdocs of xamp
• ran started and apache server
• tried accessing localhost/flarum/public

Error !!

Warning: require(C:\xampp\htdocs\flarum\vendor\composer/../flarum/core/src/helpers.php): Failed to open stream: No such file or directory in C:\xampp\htdocs\flarum\vendor\composer\autoload_real.php on line 71

and i have done composer install and update (as potential fixes and not really helped) , also asked the same question on the discord group

SOLVED had to delete vendor folder and then run composer install again.

Glad to hear you ended up getting it working! Web server setup can be a real pain, and there's frequently some filesystem permission issues or port configuration things that can be challenging. Are you able to change the source code and see that reflected on your local site?

[Adams-Ijachi]

Well so far the project is working i have created a forum and the database updates, i'm about to make a commit was able to do some refactoring(in the Index) and in Tag , i inherited Index and called the function to generate parameters. But not been able to make or actually see any visual change. ..again thanks for keeping up with me :smiley: 😄 ❤️

[askvortsov1]

Well so far the project is working i have created a forum and the database updates, i'm about to make a commit was able to do some refactoring(in the Index) and in Tag , i inherited Index and called the function to generate parameters. But not been able to make or actually see any visual change. ..again thanks for keeping up with me smiley smile heart

I suppose in this case, there shouldn't be any visual changes since it's a refactoring project :laughing:. On a side note, to test things out, disable JS in your devtools console settings temporarily; this will load the no-js version of the forum, allowing you to better see what is being generated.

[Adams-Ijachi]

OK so i'm sorry for all this commits 🙏 , but yeah made a commit to Tag and Index

[askvortsov1]

No need to apologize, we're very grateful for your work and contributions! I'll share a few comments here since they apply to both PRs:

• We generally try to restrict PRs to only the changes needed for each PR. This means that changes to things like the package-lock.json, JS dist files, and non-code whitespace changes should not be included. Additionally, we use yarn internally so no package-lock.json is needed, and our dist JS is generated automatically via GitHub actions to keep PRs small, decrease merge conflicts, and improve readability.
• When referring to extensibility, I was actually thinking that the Tag content class should inherit the Index content class, since the two are fundamentally the same view, only with Tag overriding some things (e.g. changing the request params to include the tag filter).
• If you're interested in an extra challenge, you could add a no-js view to the Subscriptions page in the subscriptions ext, which would somewhat mirror the one added by tags, except it would use a subscribed filter instead of a tags one.

Overall, great start though!

[Adams-Ijachi]

Thank You for the feedback ,greatly appreciated and about the inheriting i think i did that with this PR https://github.com/flarum/tags/pull/158 Not sure why it didn't come up here , and yep and up for a challenge.

[askvortsov1]

Thank You for the feedback ,greatly appreciated and about the inheriting i think i did that with this PR https://github.com/flarum/tags/pull/158 Not sure why it didn't come up here , and yep and up for a challenge.

It looks like instead of inheriting (extending the class via the extends keyword), you're injecting the Index class as a constructor dependency and then using it as a property. Generally speaking this is often a cleaner style of programming, but in this case, I think it would be better to stick with regular OOP inheritance.

[Adams-Ijachi]

Ok thanks , i have made the changes to tag it is currently inheriting core Index, and about removing those dist files ,i added them to gitignore but it still pushes even after clearing the cache (any tips would be great).

And about the no-js view can you provide a link to any existing (no-js view) in the project kinda struggling to understand some implementations

[askvortsov1]

Ok thanks , i have made the changes to tag it is currently inheriting core Index, and about removing those dist files ,i added them to gitignore but it still pushes even after clearing the cache (any tips would be great).

It's not about gitignore (we do want to commit them, we just do so via the GH actions script), you just shouldn't stage them when making commits. What IDE are you using?

To remove files from commits, this guide might help: https://devconnected.com/how-to-remove-files-from-git-commit/

And about the no-js view can you provide a link to any existing (no-js view) in the project kinda struggling to understand some implementations

To clarify, the no-js view IS the Content/Tags and Content/Index classes you've been editing in your PRs. These classes modify the HTML document to add various metadata (e.g. payload, title, links, etc), as well as the no-js HTML content (that's what $document->content = ...` does.

To actually see this HTML, you'll need to disable JavaScript for your browser tab: (https://stackoverflow.com/questions/13405383/how-to-disable-javascript-in-chrome-developer-tools)

[Adams-Ijachi]

ok sorry been away , i use vscode and i'll follow the steps

[askvortsov1]

ok sorry been away , i use vscode and i'll follow the steps

No rush! Thank you again for taking this initiative on, it's very appreciated :grin:

[Adams-Ijachi]

I turned off js and i can see "This site is best viewed in a modern browser with JavaScript enabled. All Discussions" but no change to either tag or index shows up there (i was just testing to see how it works)