Feature: Adds Lightbox support for the File field

[zhephyn]

Description

Adds Light box support for the file field

Fixes #1647

Checkbox

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works

[zhephyn]

@Paul-Bob I'm having trouble registering the light box controller in the application. I had initially imported from the stimulus components library. However, it seems I will have to extend it for the functionality to work.

Ideally if it works, I'm supposed to see this message logged in the browser console.

console.log("should see this message logged in console")

Tried looking at the docs but it seems i got more confused.

[qlty-cloud-legacy]

The PR diff size of 15842 lines exceeds the maximum allowed for the inline comments feature.

[qlty-cloud-legacy]

Code Climate has analyzed commit 2b11638 and detected 0 issues on this pull request.

View more on Code Climate.

[Paul-Bob]

Hi @zhephyn,

I noticed that the yarn.lock file has an unusually large number of changes. That's not ideal. I'm not sure how that happened, but it should be looked into.

Also, it seems you're registering two different things under the same name:

application.register(lightbox, LightboxController)
application.register('lightbox', Lightbox)

Additionally, the first controller registration might contain a syntax error. The identifier should be a string, like this:

application.register('lightbox', LightboxController)

Lastly, it looks like the controller isn't actually being used in the DOM, so it never connects. For the controller to initialize properly, you need to attach it to an element like this:

<div data-controller="lightbox"></div>

When this element is loaded, the lightbox controller will connect to it and start working as expected.

[qlty-cloud-legacy]

The PR diff size of 15890 lines exceeds the maximum allowed for the inline comments feature.

[zhephyn]

Think I'm gonna go with a custom light box controller instead of the one that is imported from the stimulus components library. Gonna rebase and possibly delete that yarn file with a large diff

[qlty-cloud-legacy]

The PR diff size of 15889 lines exceeds the maximum allowed for the inline comments feature.

[qlty-cloud-legacy]

The PR diff size of 6519 lines exceeds the maximum allowed for the inline comments feature.

[qlty-cloud-legacy]

The PR diff size of 6628 lines exceeds the maximum allowed for the inline comments feature.

[zhephyn]

@Paul-Bob I've successfully rebased the PR to remove the unnecessary previous commits. Now the PR, shows one commit which is related to me implementing a custom light box controller that we'll use to implement the feature as opposed to the one i had imported from the stimulus components library before.

Still though, I'm having trouble registering the controller even after making it available for the cover photo component.

As for the tests, I wanted to first make sure that the functionality is working, then I will add tests for the light box functionality.

PS: Sorry about the cluttering of the PR with code climate comments. Not really sure how to delete them to make the PR cleaner.

[Paul-Bob]

@Paul-Bob I've successfully rebased the PR to remove the unnecessary previous commits. Now the PR, shows one commit which is related to me implementing a custom light box controller that we'll use to implement the feature as opposed to the one i had imported from the stimulus components library before.

Cool!

Still though, I'm having trouble registering the controller even after making it available for the cover photo component.

What is the issue? What are you expecting to happen and is not? How are you veryfing if the controller is connecting or not?

As for the tests, I wanted to first make sure that the functionality is working, then I will add tests for the light box functionality.

We're still far from tests, let's ensure we implement the feature before adding tests for it.

PS: Sorry about the cluttering of the PR with code climate comments. Not really sure how to delete them to make the PR cleaner.

That's no problem at all.

[zhephyn]

@Paul-Bob I expect this message to be logged to the browser console to signify that the controller connected successfully.

connect() {
    console.log("Lightbox controller connected")
  }

Usually that's how I do it. Is there a better way for me to check that the controller is regsitered?

[Paul-Bob]

That is all right, and what page are you visiting and expecting the message to be present on the browser console? I moved to this branch locally and I was able to see the connect debug message on my browser console

[zhephyn]

I've been navigating to the posts show page for an individual post as every post has a cover photo. So I figured this page would be perfect for seeing the console message.

[Paul-Bob]

I think you are confusing components, the cover where you added the controller is the one rendered here https://github.com/avo-hq/avo/blob/main/app/components/avo/panel_component.html.erb#L2

And it is the big cover photo that appear on show view like this: https://main.avodemo.com/avo/resources/events/47

[zhephyn]

Not sure if I get what you're trying to say. I had added the controller to the opening div inside the cover_photo_component.html.erb
Was the controller supposed to be added to a different component file and is this not the component which renders the cover photo on the post show page?

[Paul-Bob]

Not sure if I get what you're trying to say. I had added the controller to the opening div inside the cover_photo_component.html.erb

That component is rendered when the resource has self.cover_photo configured.

Was the controller supposed to be added to a different component file and is this not the component which renders the cover photo on the post show page?

I'm unclear on where the controller is supposed to go. Based on the original issue request, I think it should be added to the file and files fields.

What I'm trying to say is that you added the controller to a component that isn't present on the post resource's show component. The cover_photo_component.html.erb is only responsible for rendering this:

class Avo::Resources::Event < Avo::BaseResource
  self.cover_photo = {
    size: :xl,
    visible_on: [:show, :index],
    source: -> {
      if record.present?
        record.cover_photo
      else
        Event.first&.cover_photo
      end
    }
  }
end

https://main.avodemo.com/avo/resources/events/47

[Paul-Bob]

field :cover_photo, as: :file, ...
field :cover_photo, as: :external_image, ...

I think I understand from where your confusion started, this fields have nothing to do with the cover component, these are just 2 fields, one as file and the other as external image, that are using the cover_photo name by coincidence.

[zhephyn]

Ohhh I get it now.
let me dig around and see if I can figure out where it's supposed to go. Gonna report back ASAP!

[Paul-Bob]

Ohhh I get it now. let me dig around and see if I can figure out where it's supposed to go. Gonna report back ASAP!

Cool!

No rush, take your time

[zhephyn]

Hey @Paul-Bob

So I've finally moved the data-action and target attributes for the lightbox stimulus controlller to the right file which renders the cover photo on the post show page. This file is the grid_item_component.html.erb.

At this point, I expected the functionality to work, but just like before, its not working. Still not sure if the controller was registered because even when i added data-controller="lightbox" to the main div of the grid_item_component.html.erb file, i expect to see a message logged to the console when this component is rendered like you had mentioned. However, that's not the case.

All in all, I'm not sure if its not working because of incorrect implementation or because the controller was never registered to begin with.

[zhephyn]

Tried this as a way of potentially fixing it but I get 2 errors related to the commands being unrecognized.

zhephyn@mail:~/Desktop/avo$ rails tailwindcss:watch
Unrecognized command "tailwindcss:watch" (Rails::Command::UnrecognizedCommandError)
zhephyn@mail:~/Desktop/avo$ rails tailwind:css build
Unrecognized command "tailwind:css" (Rails::Command::UnrecognizedCommandError)

[zhephyn]

However to address your comment from the initial review about the image being overstretched, by adding the mx-auto class to the image, we can prevent it from being over stretched, such that we have something which looks like this:

[Paul-Bob]

So, the green background is not applied when I add it.

Are you using the bin/dev command to start the server?

[zhephyn]

Yeah.

[github-actions]

This PR has been marked as stale because there was no activity for the past 15 days.

[zhephyn]

Hey @Paul-Bob, The github actions bot just marked this as stale, which may result in the PR being closed. I could try again to debug the UI tailwind issue if its cool with you.

[Paul-Bob]

I could try again to debug the UI tailwind issue if its cool with you.

The issue seems to be on your local environment. bin/dev does run css: yarn build:css --watch, so I'm not sure why the watcher is not working on your environment. You're the right person to debug and fix it on your side. Alternatively, if you want to move fast without debugging this problem, you can just run yarn build:css after each style change to rebuild and confirm the effect in the browser.

[zhephyn]

Update. Done debugging the watcher issue.

[Paul-Bob]

Update. Done debugging the watcher issue.

What was the issue?

[zhephyn]

I actually don't know. but I deleted the project locally and re-cloned it again from Github. Following that, the watcher was picking up the new classes when i ran bin/dev. Gonna now proceed to work on the styles so that we can get this merged!

[zhephyn]

Hey @Paul-Bob . Think I've made final progress on this PR. This is what the image looks like when opened in the lightbox modal.

output.mp4

Maybe one change I would propose is the use of a heroicon instead of the current <button>X</button> syntax. I had tried making this change however I ran into the error below related to helpers.svg probably since I had used it inside a partial.

web    | Information for: ActionView::Template::Error (undefined local variable or method `helpers' for an instance of #<Class:0x00007311ce7a7080>):
web    |     1: <div data-lightbox-target="modal" class="hidden fixed inset-0 z-[100] flex justify-center" data-action="click->lightbox#close">
web    |     2:   <img data-lightbox-target="image" class="rounded-lg shadow-lg mt-4 mb-4" alt="Expanded view of the image" src="" />
web    |     3:   <%= button_tag type: "button", class: "flex pt-2 ml-2", data: { action: "click->lightbox#close"} do %>
web    |     4:     <%= helpers.svg('heroicons/solid/x-mark') %>
web    |     5:   <% end %>
web    |     6: </div>
web    |   
web    | app/views/avo/partials/_lightbox.html.erb:4
web    | app/views/avo/partials/_lightbox.html.erb:3
web    | 
web    | Information for cause: NameError (undefined local variable or method `helpers' for an instance of #<Class:0x00007311ce7a7080>):

Then finally about the failing feature test, I think it could be a flaky issue because all the failing tests pass on my machine.

In case of anything, lmk and I'll poke around, otherwise it seems to me that this is close to completion.

[github-actions]

This PR has been marked as stale because there was no activity for the past 15 days.

[github-actions]

This PR has been marked as stale because there was no activity for the past 15 days.

[github-actions]

This PR has been marked as stale because there was no activity for the past 15 days.