Improve look of section navigation cards when at least one section has an image #2302

anaulin · 2024-03-14T02:21:02Z

🥔🗜️✨ Gizmo: SectionNavigation #1988

This PR refines the look of the section navigation component:

when at least one of the sections has an image an others don't, it inserts a colored image placeholder in all the cards that don't have an image
when at least one of the sections has a description text, it makes sure that all the cards have the same description height, even if they don't have a description text

Before

After

rosschapman · 2024-03-14T18:56:09Z

app/furniture/section_navigation/section_navigations/_section_navigation.html.erb

@@ -1,7 +1,17 @@
 <div class="grid grid-cols-1 gap-5 sm:gap-6 sm:grid-cols-2 lg:grid-cols-4">
+  <% has_section_images = section_navigation.rooms.any? { |r| r.hero_image.present? } %>


I don't love seeing this type of logic in templates because I like to swing toward logic-less templates generally, but...I also wouldn't elevate it to the model since it's quite view specific. Of course a ViewComponent class would be a nice place for this. (Just thinking out loud.)

The other "Rails-y" place for this is a method in a helper -- we could start a SectionNavigationHelper and add this as a predicate there.

My feeling is that this probably wants to become a ViewComponent sooner rather than later, and in that case this will make sense as a method in there.

I generally avoid having complex logic in a view, but I also think that most views in the real world will have some kind of logic in them, so I'm not religious about it. In this case I feel like this is simple enough and enough of an area that we are likely still iterate on that this is ok for now.

rosschapman · 2024-03-14T18:56:27Z

app/furniture/section_navigation/section_navigations/_section_navigation.html.erb

  <% policy_scope(section_navigation.rooms).each do |room| %>
    <%= link_to polymorphic_path(room.location), id: dom_id(room, :link_to), class: "group no-underline" do %>
      <%= render CardComponent.new(media: room.hero_image, classes: "flex flex-col justify-between h-full") do |card| %>
+        <% if has_section_images && room.hero_image.blank? %>
+          <% card.with_header do %>
+            <div class="text-6xl tracking-wider h-36 bg-amber-50 text-amber-600/25 p-4 rounded-md text-clip overflow-hidden flex">


Tailwind is...flexible. 🤣

rosschapman · 2024-03-14T18:58:48Z

@anaulin nice use of clamping!

zspencer

I'm not sure I'm sold on the amber (it makes it look a bit like a caution sign, maybe grey?) and it feels like the text is a bit large... (text-4xl perhaps is the right size?)

Anyway, I think it's fine as-is; and we can polish it up later.

anaulin · 2024-03-28T01:01:47Z

Hmm, yeah, I sort of don't love this. I was envisioning something more "pictorial" that could be auto-generated as stand-ins for missing pictures, but my result looks more like A VERY LARGE TITLE THAT SOMETIMES DOESN'T FIT.

@zspencer @rosschapman would something like this be better? Or do you like it more with the text? This version randomizes the background color for a more placeholder-y look, and removes the text:

(The height of the bottom row needs to be adjusted to match the top row with the description, but that's a different issue.)

zspencer · 2024-03-28T01:55:14Z

ooo, I like that quite a bit! The pastels are lovely!

rosschapman · 2024-03-29T00:39:16Z

@anaulin looks good. Will passing the no_padding variant option to card.with_header remove the spacing so that colored area is flush against the card container?

anaulin · 2024-03-29T01:23:46Z

@rosschapman removing that margin is one of several remaining TO-DOs in this PR (see the PR description for my full list so far)

…to the other cards. When one of the section cards has a description, make sure that all cards have uniform heights, even if that particular card does not have a description.

anaulin · 2024-04-04T01:51:29Z

@zspencer @rosschapman I've gone back and cleaned up the implementation, and updated the screenshots in the PR description. Please have a final look!

rosschapman

🌈

rosschapman · 2024-04-04T01:59:59Z

app/furniture/section_navigation/section_grid_component.rb

+      bg-rose-100 bg-sky-100 bg-teal-100
+    ].freeze
+    def image_placeholder_div
+      content_tag(:div, "", class: "h-36 #{BG_COLORS.sample} m-0 p-0 rounded-t-lg w-full")


zspencer

Very neat!

anaulin · 2024-04-04T19:32:36Z

This is in production now. I notice that on Piikup's entrance, some of the sections aren't showing an image, but are also not getting the placeholder image:

So this needs some revisiting, to make sure that the check to see if there is a hero image for a room is correct.

I also noticed:

there a lot more rooms visible in the section navigation if you log in
- it would be nice to add a little indicator to the cards to signify that a card/room won't be listed for logged-out people, maybe something like a little icon with a crossed-out eye and a tooltip; that way Space administrators could see more clearly at a glance what will be visible to regular users
we really should order the sections alphabetically (until we let users set an order, at least), right now it feels kinda haphazard

anaulin requested a review from a team March 14, 2024 02:21

anaulin force-pushed the au/refine-section-navigation-cards branch from 54c54cb to b5b73de Compare March 14, 2024 02:22

rosschapman approved these changes Mar 14, 2024

View reviewed changes

zspencer approved these changes Mar 15, 2024

View reviewed changes

anaulin force-pushed the au/refine-section-navigation-cards branch from b5b73de to af963ab Compare March 28, 2024 00:45

anaulin changed the title ~~Improve look of section navigation cards when at least one section has an image~~ [WIP] Improve look of section navigation cards when at least one section has an image Mar 28, 2024

When one of the section cards has an image, add an image placeholder …

4592429

…to the other cards. When one of the section cards has a description, make sure that all cards have uniform heights, even if that particular card does not have a description.

anaulin force-pushed the au/refine-section-navigation-cards branch from af963ab to 4592429 Compare April 4, 2024 01:42

anaulin changed the title ~~[WIP] Improve look of section navigation cards when at least one section has an image~~ Improve look of section navigation cards when at least one section has an image Apr 4, 2024

anaulin requested review from rosschapman and zspencer April 4, 2024 01:50

rosschapman approved these changes Apr 4, 2024

View reviewed changes

zspencer approved these changes Apr 4, 2024

View reviewed changes

anaulin merged commit 2092515 into main Apr 4, 2024
3 checks passed

anaulin deleted the au/refine-section-navigation-cards branch April 4, 2024 18:32

anaulin mentioned this pull request Apr 11, 2024

Fix logic for determining if a Room has a hero image (aka header image) #2348

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve look of section navigation cards when at least one section has an image #2302

Improve look of section navigation cards when at least one section has an image #2302

anaulin commented Mar 14, 2024 •

edited

Loading

rosschapman Mar 14, 2024

anaulin Mar 28, 2024

rosschapman Mar 14, 2024

rosschapman commented Mar 14, 2024

zspencer left a comment

anaulin commented Mar 28, 2024 •

edited

Loading

zspencer commented Mar 28, 2024

rosschapman commented Mar 29, 2024

anaulin commented Mar 29, 2024 •

edited

Loading

anaulin commented Apr 4, 2024

rosschapman left a comment

rosschapman Apr 4, 2024

zspencer left a comment

anaulin commented Apr 4, 2024

		@@ -1,7 +1,17 @@
		<div class="grid grid-cols-1 gap-5 sm:gap-6 sm:grid-cols-2 lg:grid-cols-4">
		<% has_section_images = section_navigation.rooms.any? { \|r\| r.hero_image.present? } %>

Improve look of section navigation cards when at least one section has an image #2302

Improve look of section navigation cards when at least one section has an image #2302

Conversation

anaulin commented Mar 14, 2024 • edited Loading

Before

After

rosschapman Mar 14, 2024

Choose a reason for hiding this comment

anaulin Mar 28, 2024

Choose a reason for hiding this comment

rosschapman Mar 14, 2024

Choose a reason for hiding this comment

rosschapman commented Mar 14, 2024

zspencer left a comment

Choose a reason for hiding this comment

anaulin commented Mar 28, 2024 • edited Loading

zspencer commented Mar 28, 2024

rosschapman commented Mar 29, 2024

anaulin commented Mar 29, 2024 • edited Loading

anaulin commented Apr 4, 2024

rosschapman left a comment

Choose a reason for hiding this comment

rosschapman Apr 4, 2024

Choose a reason for hiding this comment

zspencer left a comment

Choose a reason for hiding this comment

anaulin commented Apr 4, 2024

anaulin commented Mar 14, 2024 •

edited

Loading

anaulin commented Mar 28, 2024 •

edited

Loading

anaulin commented Mar 29, 2024 •

edited

Loading