Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prepare for ViewComponents #1749

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Prepare for ViewComponents #1749

wants to merge 11 commits into from

Conversation

nimmolo
Copy link
Contributor

@nimmolo nimmolo commented Dec 27, 2023 •
edited

  • add the gems for ViewComponents
  • add gems for the seemingly-improved EvilMartians defaults
  • add base components to the /views/components folder
  • rubocop the changes

@nimmolo nimmolo marked this pull request as draft December 27, 2023 04:34
@coveralls
Copy link
Collaborator

coveralls commented Dec 27, 2023 •
edited

Coverage Status

coverage: 94.413% (-0.2%) from 94.607%
when pulling fb0606a on nimmo-view-component
into a803d4a on main.

@nimmolo nimmolo changed the title preparation for ViewComponents Prepare for ViewComponents Dec 27, 2023
@nimmolo
Copy link
Contributor Author

nimmolo commented Dec 27, 2023 •
edited

Update: No urgency. Folder re-org in #1753

@nimmolo nimmolo marked this pull request as ready for review December 27, 2023 19:26
JoeCohen
JoeCohen reviewed Dec 27, 2023
View reviewed changes
app/helpers/application_helper.rb
@@ -165,4 +165,10 @@ def get_next_id(object)

query.result_ids[idx + 1] || query.result_ids[idx - 1]
end

# make View components easier to call
def component(name, *args, **kwargs, &block)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on how you and others feel about all-arguments forwarding,
the following should work here. Your choice:

Suggested change
def component(name, *args, **kwargs, &block)
def component(name, ...)

JoeCohen
JoeCohen reviewed Dec 27, 2023
View reviewed changes
Copy link
Member

@JoeCohen JoeCohen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it briefly locally without problems.
It looks like it may take some getting used to the conventions.

@nimmolo
Copy link
Contributor Author

nimmolo commented Dec 28, 2023 •
edited

@JoeCohen Thanks for checking it out.

Other than finding things in /views this PR doesn't change anything immediately. It occurs to me that reorganizing the views folder is kind of a separate scope and it's worth evaluating on its own, just to separate mailers from other views. I may do that PR separately (#1753), and I could keep this one on hold for when i'm actually ready to write a view component.

Converting re-usable partials and helpers to components does mean getting used to some different conventions for sure. I'm thinking to start with heavily used components where there would be an immediate benefit in performance: images, carousels, and matrix boxes.

But those would all be separate PRs, one at a time.

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

@JoeCohen JoeCohen JoeCohen left review comments

@mo-nathan mo-nathan Awaiting requested review from mo-nathan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nimmolo @coveralls @JoeCohen