-
Notifications
You must be signed in to change notification settings - Fork 17
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
front controller path/routing adjustment #687
front controller path/routing adjustment #687
Conversation
I agree with you that |
afc1935
to
eff905c
Compare
already sends this to e-five but I figured it should be here too: as for how things are currently:
something that could use help/input:
|
0e5de43
to
b381d66
Compare
current state as of writing, if that means anything:
|
b381d66
to
8f18c21
Compare
ok, giving another go at plucking out as to what was done this time:
|
Took this for a spin and it looks good to me, do you still have any outstanding work? I was going to say if the redirect stuff made things more difficult, we could consider removing it, but it seems it's sort of been reworked into allowing short form URLs, so maybe useful to keep around? (e.g. |
type: "%default_type_options%" | ||
federation: "%default_federation_options%" | ||
content: "%default_content_options%" | ||
requirements: *front_requirement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate how you find these things that can be refactored into less copy paste
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thankfully this is one of the simpler ones, yaml anchors can be quite confusing at time, especially when you also add merge keys to the mix
technically this could go a step further by using merge keys referencing first full front route definition then overwrite path
with different routes (iirc the old front routing before ui filter patch actually does this)
|
||
return $routes[$value] ?? 'all'; | ||
return match ($value) { | ||
'article', 'articles' => Entry::ENTRY_TYPE_ARTICLE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here re: appreciate the refactoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using array map to do lookup is totally fine (and it's quite the oldest trick in the book), but in here I thought that using match
express the intention better, especially when mapping multiple inputs to a single result and also handling defaults (and the fact that we now have match
at disposal)
8f18c21
to
4e3b4ea
Compare
I don't think I have any, the main problem of path has been adjusted to look somewhat like the previous ones, I think this should be ready for review/inclusion
I didn't rework the redirect stuff so much that I just split them in two controllers that also encodes/indicates the context it should be used in (rather than having to note that |
This comment was marked as resolved.
This comment was marked as resolved.
4e3b4ea
to
2be5001
Compare
good point, added the suggested patch, that does bother me a little to be honest, I kinda glossed over the navbar part, though that's likely because I also have another plan to try and push the condition in templating front link (the block you edited) down to the |
2be5001
to
2f72b55
Compare
the new filter ui changes also introduces a couple more filtering parameters in the form of controller path params, for threads/entries listing it looks fine and relatively staightforward, but for microblog posts listing, what used to be a simple `/microblog` is now something like `/all/hot/∞/all/all/microblog`, which hinders the user experience on url readability and memorability compared to the previous scheme, for those who still care this changes tries to adjust the path/routing of these front controller especially for the microblog part, to make it more simpler and could omit some default filter parameters if they aren't set, similar to the previous path scheme note that this patch main focus is just the path routing, there's probably more to adjust/polish in front controller and related areas but those would likely be a separate follow up patch
Co-authored-by: e-five <[email protected]>
2f72b55
to
8fb2fff
Compare
the new filter ui changes also introduces a couple more filtering parameters in the form of controller path params, for threads/entries listing it looks fine and relatively staightforward, but for microblog posts listing, what used to be a simple
/microblog
is now something like/all/hot/∞/all/all/microblog
, which hinders the user experience on url readability and memorability compared to the previous scheme, for those who still carethis changes tries to adjust the path/routing of these front controller especially for the microblog part, to make it more simpler and could omit some default filter parameters if they aren't set, similar to the previous path scheme
note that this patch main focus is just the path routing, there's probably more to adjust/polish in front controller and related areas but those would likely be a separate follow up patch
some of the front path routing that I have in mind, based on on previous scheme:
/{subscription}/{content}/{sortBy}/{time}/{type}/{federation}
--/sub/microblog/newest/1d
/{subscription}/{sortBy}/{time}/{type}/{federation}
--/mod/commented/1w
(*)
/{content}/{sortBy}/{time}/{type}/{federation}
--/microblog/newest
/{sortBy}/{time}/{type}/{federation}
--/top/6h
(*)
/m/{magazineName}/{content}/{sortBy}/{time}/{type}/{federation}
--/m/playground/microblog/active
/m/{magazineName}/{sortBy}/{time}/{type}/{federation}
--/m/random/newest
(*)
path components
sortBy
and after shouldn't showed up if using default filter valuespaths marked with
(*)
could be later used for combined views default/short entry point when the time comes? right now they should be alias for whencontent
param isthreads
i.e. threads/entries front viewtype
andfederation
may be able to be pulled out into query params, esp.type
where it currently only make sense in the context of threads listing, as we don't do such categorization for microblog posts