-
Notifications
You must be signed in to change notification settings - Fork 440
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.
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
Add support for multiple formats #2079
Merged
Merged
Changes from 21 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
698e991
add test case
joelhawksley 71f4adc
two tests remaining
joelhawksley d356ce8
first time all green
joelhawksley 7c5cde5
Merge branch 'main' into multi-format-component
joelhawksley 10d68ee
Fix final line endings
github-actions[bot] 229aaab
add docs, changelog, test helpers
joelhawksley 9560ca0
Fix final line endings
github-actions[bot] 9640e67
streamline compiler method generation
joelhawksley 4bacfd9
simplification
joelhawksley eee9aec
refactor to remove index usage
joelhawksley 1331e41
clearer control flow
joelhawksley ac048cb
lint
joelhawksley 1170071
Merge branch 'main' into multi-format-component
joelhawksley 7bbba51
md lint
joelhawksley c44b5b3
remove remaining hardcoded formats
joelhawksley e46c231
Update lib/view_component/base.rb
joelhawksley 955a52e
Merge branch 'main' into multi-format-component
joelhawksley 6fb636a
remove unnecessary `inspect`
joelhawksley 87978c8
remove unused `identifier`
joelhawksley b55a94a
inline single-use method
joelhawksley 61a6d61
add safe navigation
joelhawksley e77aecd
consolidate template collision error messages to include format
joelhawksley 9777f38
add backticks around variant name in error
joelhawksley 69a377f
compiler should check for variant/format render combinations
joelhawksley 0270cba
Merge branch 'main' into multi-format-component
joelhawksley e0f057f
standardrb
joelhawksley File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Hello, CSS! |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is getting pretty complex, is this something we could break into more methods, or perhaps a separate class that's responsible for returning template objects that define the relevant methods? Like
name
,method_name
,format
, etc.?It might also make the testing story a bit easier and more comprehensive since we don't have to couple the compiler and template logic as heavily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh man, all I wanted to do while writing this PR was refactor this entire file, but I held back. I'm happy to go down that route, but maybe it would be best to do as a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah, the compiler has gotten a bit gnarly! Part of the reasoning was that I had a hard to following the code in here (but I also didn't have a ton of time to dedicate to reading it) so I thought refactoring might make it easier to follow the changes.
One strategy I've done in the past for changes like this is refactoring the PR on-top of the new behavior, backporting it while removing the new functionality as a separate PR (so 1-to-1 functionality wise), then re-adding those initial refactor changes on-top. It's a bit roundabout, but I found it helps reduce churn and makes the changes easy-to-follow.
I'm also fine with a follow-up, but I'd also say I haven't reviewed this code in depth yet if we wanted to go that route. 🙂