Skip to content

Conversation

@nshki
Copy link
Contributor

@nshki nshki commented Feb 17, 2021

Summary

First of all, I want to thank you all for this lovely gem. We have recently started adopting it over at Litmus and we are thrilled with what it enables our team with.

Something minor that we've noticed is that when generating components with the --preview flag, the preview file isn't placed in a custom preview path directory that we've defined.

e.g. With the following configuration:

# config/application.rb

module MyProjectName
  class Application < Rails::Application
    # Initialize configuration defaults for originally generated Rails version.
    config.load_defaults 6.0

    # Settings in config/environments/* take precedence over those specified here.
    # Application configuration can go into files in config/initializers
    # -- all .rb files in that directory are automatically loaded after loading
    # the framework and any gems in your application.

    config.view_component.preview_paths << Rails.root.join("spec", "components", "previews")
  end
end

,,,we'd love for the file to be placed in spec/components/previews/ when running something like:

$ bin/rails generate component Test --preview

I realized that the preview_paths config option could possibly have more than one path, so this PR only places the file in an overridden path if and only if one exists.

@joelhawksley joelhawksley requested a review from Spone February 17, 2021 14:58
@joelhawksley
Copy link
Member

Thanks for the PR @nshki! Might you be up for reviewing this generator-related change @Spone?

@Spone
Copy link
Collaborator

Spone commented Feb 17, 2021

Looks good to me. I'm just wondering if it's better to disable this behavior when there are several preview_paths, or to always use the first (or last) of the preview_paths.

@nshki
Copy link
Contributor Author

nshki commented Feb 17, 2021

Oh, good point @Spone. The current behavior of this PR generating at the default path when multiple preview_paths doesn't quite make sense. I'm leaning toward disabling this behavior altogether if multiple exist. It seems safer to not be presumptious about how teams might be structuring where preview files are stored.

e.g. (Spitballing here.) One use case I could think of is if a team wanted previews within a Sidecar directory structure, it would get annoying if new previews kept getting generated in an unrelated component directory.

@Spone
Copy link
Collaborator

Spone commented Feb 17, 2021

I'm having a hard time imagining the uses cases for this this feature, so let's stay on the safe side and stick with what you suggest: use the configured path only if there is only one defined.

Spone
Spone previously approved these changes Feb 17, 2021
@nshki
Copy link
Contributor Author

nshki commented Feb 17, 2021

Just to clarify, would you like me to disable the behavior if more than one path is defined in this PR? Wasn't sure if the PR approval meant good to merge as is.

@Spone Spone self-requested a review February 17, 2021 23:25
@Spone
Copy link
Collaborator

Spone commented Feb 17, 2021

Just to clarify, would you like me to disable the behavior if more than one path is defined in this PR? Wasn't sure if the PR approval meant good to merge as is.

Yes I think it's a good idea to disable the behavior when more than one path. Can you push this to this PR?

@Spone Spone dismissed their stale review February 17, 2021 23:25

Still one change to do

@nshki
Copy link
Contributor Author

nshki commented Feb 17, 2021

Just to clarify, would you like me to disable the behavior if more than one path is defined in this PR? Wasn't sure if the PR approval meant good to merge as is.

Yes I think it's a good idea to disable the behavior when more than one path. Can you push this to this PR?

Will do!

We want to disable preview files getting generated in the case that more
than one preview path exists.
@nshki
Copy link
Contributor Author

nshki commented Feb 18, 2021

Good for review again. 👍🏻 Had to explicitly set the preview paths to an empty array for other preview-related tests since the dummy application has a handful set by default.

@BlakeWilliams BlakeWilliams merged commit 8304cbf into ViewComponent:main Mar 1, 2021
@BlakeWilliams
Copy link
Contributor

Thanks for the contribution! ❤️

and thank you @Spone for the review! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants