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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup #1

Merged
merged 8 commits into from
Apr 30, 2021
Merged

Cleanup #1

merged 8 commits into from
Apr 30, 2021

Conversation

Ralkage
Copy link
Contributor

@Ralkage Ralkage commented Apr 29, 2021

composer.json

  • I removed the require-dev and autoload-dev keys and values because you don't really have any PHP code other than what's in your extender.php file. With your extension, you don't really need PHPUnit tests since you mostly added JS-related code.

  • For the extra > flarum-extension > category key, you have three types of extension categories that you can specify and if left empty, then it defaults to "feature". To me, it is good Flarum-related practice to add which category this extension belongs under so that other devs or even normal users know that it is either a feature, theme, or "language" pack (ref: see the last item here https://docs.flarum.org/extend/update-b16.html#frontend).

  • I left the autoload key alone because I wasn't sure whether or not you had planned to add any PHP code in the future; you don't really have a src (source) folder right now to work with so you can easily remove that key if you so choose and don't expect to add any additional PHP code.

  • As for the flagrow key, this is completely optional and given that Flagrow is now Extiverse essentially, you can choose to either leave this key alone and just add a value, rename the key to "extiverse" since Luceos says that is supported and also adding a value, or simply remove it altogether. This key is specific to Extiverse on the Extiverse-related extension page and whether or not you have a Flarum discussion to tie your extension to so that Extiverse adds a button next to the git button to link to it. With any extensions that I work with personally and in FoF, I've started to remove this key in favor of the support > forum key since Flarum supports these keys in each extension's settings page. I've added this along with a support key and added a value to the flagrow key for you. When adding a value, which is typically the Flarum discussion for your extension, you can simply reference the id of the discussion in the URL so that in the event you change the name of the discussion, the link won't be affected (https://discuss.flarum.org/d/27023).

  • I've also formatted this file for JSON, moved some sections around for easy redability, and added a support key with values linked to your github repo and extension discussion on Flarum Discuss :)

extend.php

  • I removed the unused extenders, the extra space under use Flarum\Extend;, and removed the copyright year so in the future, all you have to do is updated the year in your LICENSE.md file.

JS files:

  • I removed the unused admin.js files respectively since you didn't really add any extension settings to work with. You can always add this back in the future in the event you want to add permissions or even a way to disable the floating animation. From the JS folder, I removed the admin.js file which pointed to ./src/admin, I also removed the dist files that were compiled, and removed the ./src/admin folder entirely.

  • As mentioned in the "Good practices" section, I've also taken the liberty of formatting your JS with prettier. I also dropped the copyright header from your index.js file because most Flarum extension developers add that to their PHP file headers instead 馃檪

Resources:

  • Locale:
    • With your locale/translation file, I simplified it a bit to re-use core translation keys in your JS with the only downside being that if core changes this translation key in a future release, it will just spit out the translation key name instead of it's value (ref: see https://docs.flarum.org/extend/i18n.html#how-flarum-translates). I'll leave you to decide whether or not you like my approach or if you would rather use your own translations entirely and use ref keys from core instead.

LICENSE.md

  • I added the current year as well as your name as the copyright holder since those values weren't filled out.

README.md

  • I moved your screenshot to it's own section to make things look tidier and easier to read.

Good practices:

  • Whenever you are writing JS code and feel you are ready to build for prod, it is good practice to install a linter/formatter or IDE extension to make sure your code is nice and tidey. Fortunately with the Flarum CLI, you get the prettier dev dependency added after you generate your extension boilerplate for the first time :) Once you run npm i to install all the packages/modules needed for your extension, you have the npm run format-check command that will check if any formatting is needed for your JS code and simply npm run format to allow prettier to format your JS and write to any files that it suggested code formatting. I typically run the formatter right before I run npm run build so I know my extension is neat and ready for a release but as of lately since using VSCode, I use the Prettier extension.

  • When you no longer need a component, helper, or module, try to make it a habit to remove unused imports so not only does your code look cleaner, but other devs can also read your code easier. This goes the same for comments for functions/methods/ lines of code that are not self-describable.

@Ralkage Ralkage changed the title Fix Cleanup Apr 29, 2021
@justoverclockl justoverclockl merged commit 6c14c1b into justoverclockl:main Apr 30, 2021
@@ -37,12 +26,12 @@ app.initializers.add('justoverclock/flarum-ext-welcomebox', () => {
m('div', { class: 'iconbadge' }, listItems(user.badges().toArray())),
m('.ulwb', { class: 'contentwb' }, [
m('li', [
m('label', { class: 'textinfo' }, app.translator.trans('flarum-ext-welcomebox.forum.npost')),
m('label', { class: 'textinfo' }, app.translator.trans('core.forum.user.posts_link')),
Copy link
Contributor

@rob006 rob006 Apr 30, 2021

Choose a reason for hiding this comment

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

This change is wrong. You should not use core translations directly. You should use your own key (as before), but reference core translation in translation file (as => core.forum.user.posts_link - in this way translators have more flexibility and can translate this phrase differently than in generic phrase used by core (context matters). This is explained in docs: https://docs.flarum.org/extend/i18n.html#reuse-translations-not-keys

Copy link
Contributor Author

@Ralkage Ralkage Apr 30, 2021

Choose a reason for hiding this comment

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

Thanks @rob006, I'll get that fixed up in another PR; that pretty much clears up any questions/concerns about translations (and what was said in the docs).

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.

None yet

3 participants