-
Notifications
You must be signed in to change notification settings - Fork 1
FederationMissingNotice in Collection component not fitting and redundant #100
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
Comments
See the following comment: #102 (comment) A change from "list" to "data" should make it work. Switching to list items makes sense. |
With the changed text and aligned layout it looks like this: IMO the new text makes it okayish, but not great. Is the collection metadata really "incomplete" just because a backend is not available? Why does the rendering of an online/offline report depend on whether all of the federation backends report the collection I'm currently viewing? Why is it a FederationNotice with a "Federation" heading when the content is not really about the fact that this is on a federation but about (limited) availability within that federation? IMO naming is not ideal in several places and the whole concept could be restructured (hence my initial writing in #101), but I guess we'll just keep it as it is now. |
* Fix typos causing code to fail * Apply same logic to fed:missing as to fed:backends * Hide notice if missing backend is irrelevant * Prevent FedMissingNotice in non-federation context I returned `true` too eagerly :D When it's not a federation context, of course supportedBy is undefined, so affectedByMissing became true. The Notice didn't render anyway because of further checks in the component itself, but it was added to the DOM and that should be avoided too. * Hide notice if `missing` is empty array * Unify fed notice layouts and improve their texts (see #100, #101) * Apply suggestions from code review (cleanup and style) Co-authored-by: Matthias Mohr <webmaster@mamo-net.de> * Change texts of federation notices (see #100, #101, #102) --------- Co-authored-by: Matthias Mohr <webmaster@mamo-net.de>
Fair enough, this can be improved in the future for sure. A note with regards to the example though. The example looks pretty "bad" because it only lists EODC once in both components. Effectively this would basically never happen. If it's only hosted on a single backend and the data is incomplete, it means the data would not get delivered at all, so this would already be caught in the collection list by stating the EODC backend is offline and AUT_DEM would never occur in the list. So to make your example more realtistic, there would need to be at least two backends of a three backend federation in the first box or the box would not be shown at all because all three backends support it. So in most cases you'd only see one of the boxes. |
The Collection component already has the FederationMissingNotice implemented, but I don't think it's fitting here because of the wording ("the following list"). While that could be fixed, maybe we don't need the FederationMissingNotice at all, as the "offline" badge in the FederationNotice already conveys the necessary information:
I suggest to remove the FederationMissingNotice here and will do so if nobody says something else in the next day or two.
(Also, I will align the styling so that they both display the list as an
<ul>
.)The text was updated successfully, but these errors were encountered: