-
Notifications
You must be signed in to change notification settings - Fork 505
Several fixes to RSS component and object lists #4217
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
Conversation
* RSS button displayed on search result pages * RSS button displayed on Recent Items component * RSS button NOT displayed in other components like top-level community, etc. * RSS component tracks sort option changes
If a valid, complete SortOptions is passed to the pagination component showRSS, it will be used instead of the underlying sortOptions, otherwise the pagination context sortOptions will be used.
89caf2e to
6f878e5
Compare
|
@tdonohue i still have some new tests to write, but requesting an early review from you now to get your input on my revised approach - this will allow the button to still be displayed in top level comm list, but also keeps my earlier work to track parent pagination sort options. If you generally approve, I'll start on tests and I'd appreciate review by another angular dev too, to check my implementation |
tdonohue
left a comment
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.
@kshepherd : Gave this a test & review today. Overall, I like the approach you've taken, and the code looks good (I would appreciate it if @alexandrevryghem could give it a quick look too).
I can also verify now that the RSS button is working properly & that it's behavior changes to match the current sort order or number of results. I like that it also now appears for search results too, so that you can "subscribe" to a search via RSS.
But, I noticed a few things that I'd like us to consider fixing/changing:
- I don't like the fact that two RSS buttons now appear on the homepage when recent submissions are enabled. They are simply duplicative as they always will have the same sort order & number of results (there's no ability to reorder the homepage). I'd be OK with the button appearing in either location, but not both.
- One option here is to just document that the RSS button only appears on the homepage if recent submissions are enabled (and only have it appear there).
- I noticed with this PR that the RSS button has accidentally disappeared from all Community/Collection pages (on the default "Search" tab). It's appearing on those pages on the demo site, but is gone when I use this PR.
Beyond that, everything else looks good to me.
|
thanks @tdonohue !
The button was showing for search results previously, iirc, (that's actually how this issue got introduced I think) but it wasn't tracking changes to the sort options fully. |
app-routes previously set this but it now needs to be set in collection-page-routes and community-page-routes respectively.
This can be added in easily with: ` <ds-rss [sortConfig]="sortConfig"></ds-rss>`
|
@tdonohue button should now display on collection and community pages - the I also removed the ds-rss from the recent items template, so it won't display there. After noticing the name of that variable more (which is a more broad enable/disable based on route, rather than something overridden in the component inputs themselves), I wondered if my |
tdonohue
left a comment
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.
👍 Thanks @kshepherd ! This now looks good to me & works well. Regarding your question about the showRSS input, I don't mind the current name because it is deciding which components are going to show this RSS icon.
I did find one tiny thing below that might need cleanup before merger.
src/app/home-page/recent-item-list/recent-item-list.component.ts
Outdated
Show resolved
Hide resolved
|
Thanks @kshepherd ! Looks good! |
References
Description
The RSS button was not keeping track of sort options in any case (including search result pages). It was instead using default pagination.
This PR adds a new
showRSSinput for object collection, object list, and pagination components.The input takes type
SortOptions | boolean, allowing the syndication feed button to be simply enabled or disabled with a boolean, or have the parent paginationsortOptionsoverridden with a customsortOptions(for example, we can set "date descending" sort options for an RSS button in the top level community component, even though the pagination for top level community results is "title ascending").If a null, undefined or otherwise invalid or incomplete
sortOptionsis passed, the pagination component will pass its ownsortOptionsto the RSS button.The default value for this input in all implementing components is
false.Search results component and Recent Items components both set this to
true.The component will now update the RSS link on a change to the sort options in a search page, and uses the correct pagination/sort options for the calling component (recent items, search, etc), or the custom sort options for e.g. top-level community page
Rather than my earlier work which moved the RSS button, this will now show 2 buttons on the homepage by default. I am happy to remove the recent submissions one in this PR so there is a less obvious change.
I tend to like leaving it, because it means that even if top-level community is further customised or disabled by a repository, there is still a relevant, in-context syndication feed for the recent submissions component.
(consider that #4172 says "the info are not the same as on the homepage" -- it refers to the recent submissions here, not community names!)
Instructions for Reviewers
Without this PR applied, note the difference in Recent Items and Search Results in the displayed UI lists, vs the RSS feeds when the button is clicked
In search pages, note that the sort is still Title Ascending after the search sort options are changed
With this PR applied, try the same tests. The button on the homepage will now be displayed near Recent Submissions and the top-level community list.
Note: The additional inputs to object list, object collection components were a bigger change than I initially anticipated, and I don't think it should be breaking to any custom themes as there is a safe default set everywhere, but I want to note it just in case this moves it up from being a very simple bug fix.
TODO: If this approach is generally approved, I will write unit tests to ensure expected sort options in the ds-rss component
I may need to write some additional tests.
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)