Add empty state for /developers

[bensheldon]

Connects to #472.

I used the existing EmptyStateComponent to structure the content. Is the language and icon appropriate?

Pull request checklist

• My code contains tests covering the code I modified
• I linted and tested the project with bin/check
• I added significant changes and product updates to the changelog

[joemasilotti]

Looking good! I requested some small copy and design tweaks.

Other than that, I don't love the inline logic for dynamically setting the copy. Let's create a new component that inherits from EmptyStateComponent and move the logic into the new component.

[bensheldon]

@joemasilotti I believe the best change is to add a new argument to the existing EmptyStateComponent, called unseeded_body: and move the logic into the existing component. Mainly because I liked keeping the translations relative to the view template (rather than the ViewComponent); and maybe some other EmptyStateComponents would be able to use that functionality too.

If you don't like that, I can make a EmptyDevelopersComponent and push the logic inside of there.

I was unable to test that functionality though. When I stub out the Rails.env, for some reason a Stripe API request gets dispatched 🤷

# test/components/empty_state_component_test.rb

class EmptyStateComponentTest < ViewComponent::TestCase
  # ...
  test "Renders title, unseeded_body and icon" do
    title = "No notifications"
    body = "Nothing to see"
    unseeded_body = "Seed the database"

    User.destroy_all
    Rails.stub(:env, ActiveSupport::StringInquirer.new('development')) do
      render_inline(EmptyStateComponent.new(title:, body:, unseeded_body:, icon: "icons/brands/github.svg"))

      assert_selector("h3", text: title)
      assert_selector("p", text: unseeded_body)
      assert_selector "svg[title='GitHub logo']"
    end
  end
end

[joemasilotti]

EmptyStateComponent is generic and shouldn't know anything about what "seeds" are. Subclasing allows us to add some more specific logic without inflating that class.

Instead of stubbing Env we can inject the environment via an initializer.

def initialize(show_unseeded_message:) 

Developers::EmptyStateComponent.nee(show_unseeded_message: Rails.env.dev?)

[bensheldon]

@joemasilotti Thanks for the feedback! I added a new Developers::EmptyStateComponent that sub-renders the initial EmptyStateComponent. I did that because I didn't like changing the initialize method signature of a subclass (Liskov Substitution).

Observation: I'm not sure if where I ended up is far from where you would have gone yourself, but it does seem like there's a lot more lines of code just to rewrite the initial ternary in the template by introducing a new component. I don't have visibility into opportunities for reuse of the component though, so if there are future plans for lots of Developers views, I could see it being worth it.

[joemasilotti]

Observation: I'm not sure if where I ended up is far from where you would have gone yourself, but it does seem like there's a lot more lines of code just to rewrite the initial ternary in the template by introducing a new component. I don't have visibility into opportunities for reuse of the component though, so if there are future plans for lots of Developers views, I could see it being worth it.

Apologies, my quickly written response earlier wasn't very clear. I meant to only pass in the environment to the View Component and then do the querying and dev check inside the component.

It's not documented (yet), but all of the components inside a domain folder are not meant to be reused outside of their specific application. They should be treated like glorified partials (with testing). The components that live at the root level of app/components/ are meant to be reused and shouldn't have knowledge of any (or very little) domain models.

(I'm hoping to document some of this in app/components/README.md in the near future.)

So for this component I would rather have it know how to query for zero developers if there are no results and if we are in the development environment. That way we can pass only the environment flag or nothing at all in.

Does that make sense?

[bensheldon]

@joemasilotti I think it makes sense. I made changes inline with my understanding. Hopefully it meets your needs 😄

More broadly, I think this is just some unsurfaced complexity. Thinking through it:

• Seeded/unseeded state is global to the application.
• The EmptyStateComponent (or even the new Developer::EmptyStateComponent) doesn't have any insight into the conditions/state under which it is rendered or not (e.g. it doesn't have access to which developers are empty and if a filter has been applied).
• Therefore, I think the component is going to be debatably messy in code because of the mismatch between having to both access global state, but also not consider local/specific state, but be a highly localized component.
• ...and thus if this set of changes doesn't meet your expectations, my recommendation would be to use a banner/statusbar that was more application layout-level because I think that's going to more closely align the visual-data-code hierarchy.

[joemasilotti]

...and thus if this set of changes doesn't meet your expectations, my recommendation would be to use a banner/statusbar that was more application layout-level because I think that's going to more closely align the visual-data-code hierarchy.

That's an excellent point! I'd love to change the scope of this PR to handle just that, if you have the appetite.

1. Roll back to using the EmptyStateComponent directly and ignore any context around environment, number of developers, and filtering status.
2. Create a new component, Developers::UnseededWarning (or something), that renders like Developers::NewFieldsComponent (but red and with the exclamation Heroicon) alerting the user to seed their database. Only show this banner in development and if there exactly 0 developers in the database.

How does that sound?

[bensheldon]

@joemasilotti sounds good 👍 I'll slightly rewrite the issue and let's split it into 2 PRs. That way we can land the relatively simple empty state and get that benefit to users, and then land the development message.

[joemasilotti]

Good idea. Let me know when this one is ready for another review.

[bensheldon]

@joemasilotti this is ready for another review 😄

[bensheldon]

I also just pushed up #494 for db:seed hinting. I can rebase it once this PR goes in.

[joemasilotti]

Two tiny suggestions and we are good to merge!

[joemasilotti]

Awesome! Thanks for the contribution and feedback on this one. I like where it ended up much better than what I originally imagined. 💪