No slug? Then no '-' separator! #1351
Conversation
If there is no slug for the discussion then the '-' separator is redundant and should not be used.
|
EDIT: ignore me |
|
I was not sure wether if it would be acceptable (or not) to push compiled files so I did not push them to this repository but if this is a requirement then just let me know... and I will do it! (Please, note that there is also 2 .php files committed) |
There was a problem hiding this comment.
Apart from this one change, this looks good to me! No need to include compiled JS. Thanks @johannsa :)
js/forum/src/initializers/routes.js
Outdated
| @@ -35,7 +35,7 @@ export default function(app) { | |||
| */ | |||
| app.route.discussion = (discussion, near) => { | |||
| return app.route(near && near !== 1 ? 'discussion.near' : 'discussion', { | |||
| id: discussion.id() + '-' + discussion.slug(), | |||
| id: discussion.id() + (discussion.slug().trim() ? '-' + discussion.slug() : ''), | |||
There was a problem hiding this comment.
Please extract discussion.slug() as a variable before the return statement (ie. const slug = discussion.slug())
|
Hi, @tobscure! I will do this on Sunday, when I am back from a laptop free
trip.
I understand CI does not take care of compiling files... Should I commit
the compiled files or can I assume they will be compiled prior to release?
Thanks.
El El mar, 30 ene 2018 a las 8:15, Toby Zerner <[email protected]>
escribió:
… ***@***.**** requested changes on this pull request.
Apart from this one change and the JS recompilation, this looks good to
me! Thanks @johannsa <https://github.com/johannsa> :)
------------------------------
In js/forum/src/initializers/routes.js
<#1351 (comment)>:
> @@ -35,7 +35,7 @@ export default function(app) {
*/
app.route.discussion = (discussion, near) => {
return app.route(near && near !== 1 ? 'discussion.near' : 'discussion', {
- id: discussion.id() + '-' + discussion.slug(),
+ id: discussion.id() + (discussion.slug().trim() ? '-' + discussion.slug() : ''),
Please extract discussion.slug() as a variable before the return
statement (ie. const slug = discussion.slug())
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1351 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AANGzq_P7IT5lGlmcaBNgsJKcUkst3qzks5tPs-QgaJpZM4Rv50i>
.
|
|
Please do not commit the compiled JS files, we will take care of that on our end :) |
There was a problem hiding this comment.
Changes as requested by @tobscure have been made, thanks!
General question, though: should we maybe move this calculation onto a further attribute computed in the Discussion model on the server-side? Then we don't need to re-create the logic twice.
Please, see flarum#1351
If there is no slug for the discussion then the '-' separator between the discussion id and the discussion slug is redundant and should not be used.
This would allow to have numeric only permalinks for discussions if slugs are empty.