[F] Improve Journal + Project Collection Discoverability#3958
Conversation
|
Thanks, Lauren! |
7283dd7 to
a4924dc
Compare
14776cc to
6593513
Compare
1bab493 to
9d8893f
Compare
dananjohnson
left a comment
There was a problem hiding this comment.
@1aurend @jen-castiron this is looking good so far! I'll think a bit more about the project collections thumbnail, but here's some feedback to address in the meantime.
client/src/frontend/components/entity/Collection/patterns/Journals.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@jen-castiron a couple changes here:
- The collecting toggle shouldn't show up for project collections. They can't actually be collected.
- Can we show some metadata here for journals and project collections? Projects show "Updated at…", which might be nice at the very least.
There was a problem hiding this comment.
There was a problem hiding this comment.
The more I look at EntityMetadata the more I wonder if it needs a bit of a refactor. There's a lot of shared stuff between entity types, especially UI-wise, but we might want to rethink getMetadataProps before adding even more logic to it.
There was a problem hiding this comment.
I see what you mean! I'll take a look at that wrapper and component and see what we could do to make it a little more generic.
There was a problem hiding this comment.
We talked about this in FE confab - changes have been pushed! If we want more information on journal / project collection thumbnails, we'll need to add more data to the entity properties.
| let routeName = "frontendProjectDetail"; | ||
|
|
||
| if (entity.type === "journals") { | ||
| routeName = "frontendJournalDetail"; | ||
| } else if (entity.type === "projectCollections") { | ||
| routeName = "frontendProjectCollection"; | ||
| } |
There was a problem hiding this comment.
Since this is a global generic component, I wonder if we should just make the route name a prop.
There was a problem hiding this comment.
I understand your concern, but this component isn't truly generic. It is generic to entities, which have a type and a specific route name. (The component originally went to "frontendProjectDetail", no matter what entity you gave it.) Since this component gets the full entity object with type, it seemed unnecessary to made a developer pass in the route name.
I could see passing in the route name if there's a chance we want the preview to go somewhere other than the entity type's detail page.
Edit: I'm taking a look at this as part of the EntityThumbnail / EntityMetadata refactor
|
@dananjohnson @1aurend |
I don't love nested ternaries but am not an absolutist about it. I think my rule would be: nesting once is fine, but if you're doing more than that, find another solution like a switch. Otherwise it just gets too hard to read. |
While I agree there's a limit, I end up disabling this lint rule a lot. I'm all for removing it. |
98a02a9 to
bba6fe3
Compare
* Also allow filtering subjects to used by journals
- Update route in EntityThumbnail for journals - Update when placeholder should be shown on journals list - Add journal subjects to store
8b58e56 to
458f390
Compare




@dananjohnson @jen-castiron here's the branch with the updated library route structure. For project collections, I added redirects at the old paths to keep existing bookmarks working and updated the redirects in and out of admin.