[WIP] Introduce Literal properties #2893
Conversation
|
Code Climate has analyzed commit 4240d80 and detected 0 issues on this pull request. View more on Code Climate. |
Gemfile
Outdated
| # TODO: Move this into the Gemspec | ||
| gem "literal", path: "../literal" |
There was a problem hiding this comment.
I will cut a release on RubyGems before this PR is ready to merge and move this up to the Gemspec.
| prop :label, _Nilable(String), reader: :public do |value| | ||
| value || I18n.t("avo.actions") | ||
| end | ||
| prop :size, Symbol, default: :md, reader: :public |
There was a problem hiding this comment.
I’m not sure what the union of sizes is for this component. I couldn't see that this option is even being used at the moment.
| prop :label, _Nilable(String), reader: :public do |value| | ||
| value || I18n.t("avo.actions") | ||
| end |
There was a problem hiding this comment.
If I18n.t("avo.actions") will never be nil, we can actually make the type of this String instead of _Nilable(String).
| @user = user | ||
| end | ||
| class Avo::SidebarProfileComponent < Avo::BaseComponent | ||
| prop :user, _Nilable(_Any), reader: :public |
There was a problem hiding this comment.
I couldn’t figure out what to make this type. It seems to be called with nil in some of the tests. Otherwise it’s a User record, but it doesn't seem to be specifically an "Avo-User". So maybe it should be an ActiveRecord::Base? Can we be more specific? Avo::BaseResource or something, maybe?
| end | ||
| class Avo::DividerComponent < Avo::BaseComponent | ||
| prop :label, _Nilable(String), :positional, reader: :public | ||
| prop :args, Hash, :** |
There was a problem hiding this comment.
I don’t think this is needed, but it was allowed before so didn’t want to break anything.
| def actions | ||
| def filter_actions | ||
| if @exclude.present? | ||
| @actions.reject { |action| action.class.in?(@exclude) } | ||
| @actions.reject! { |action| action.class.in?(@exclude) } | ||
| elsif @include.present? | ||
| @actions.select { |action| action.class.in?(@include) } | ||
| else | ||
| @actions | ||
| @actions.select! { |action| action.class.in?(@include) } |
There was a problem hiding this comment.
This seemed to be doing quite a bit of work each time it was called so I thought it might make sense to do it once and store it back to the @actions ivar.
| prop :stacked, _Boolean, default: false do |value| | ||
| !!value | ||
| end |
There was a problem hiding this comment.
This seemed to be used as a boolean, but was true | nil instead of true | false. For now, I’ve made it coerce input into a boolean, but it might be better to fix the callsites to send true | false instead of true | nil.
| # Override on the declaration level | ||
| return @stacked unless @stacked.nil? | ||
|
|
||
| # Fetch it from the field | ||
| return field.stacked unless field.stacked.nil? | ||
|
|
||
| # Fallback to defaults | ||
| Avo.configuration.field_wrapper_layout == :stacked | ||
| @stacked || field.stacked || Avo.configuration.field_wrapper_layout == :stacked |
There was a problem hiding this comment.
nil and false are both falsey, so I think we can just make this a simple OR fallback.
| prop :label_for, _Nilable(String) | ||
| prop :args, Hash, :** | ||
|
|
||
| def after_initialize | ||
| @action = field.action |
There was a problem hiding this comment.
Unless field.action is slow, there's probably no need to assign an instance variable here. This might be better as a method or delegation, e.g.
delegate :action, to: :@field
| prop :form, _Nilable(ActionView::Helpers::FormBuilder), reader: :public | ||
| prop :full_width, _Boolean, default: false, reader: :public | ||
| prop :label, _Nilable(String) # do we really need it? | ||
| prop :resource, _Nilable(Avo::BaseResource), reader: :public |
There was a problem hiding this comment.
I’m not convinced all these should be nilable, but I needed to make these ones nilable to get the tests to pass.
| prop :dash_if_blank, _Boolean, default: true, reader: :public | ||
| prop :data, Hash, default: -> { {} } | ||
| prop :compact, _Boolean, default: false, reader: :public | ||
| prop :help, _Nilable(String) # do we really need it? | ||
| prop :field, Avo::Fields::BaseField, reader: :public | ||
| prop :form, _Nilable(ActionView::Helpers::FormBuilder), reader: :public | ||
| prop :full_width, _Boolean, default: false, reader: :public | ||
| prop :label, _Nilable(String) # do we really need it? | ||
| prop :resource, _Nilable(Avo::BaseResource), reader: :public |
There was a problem hiding this comment.
There’s a lot of reader: :public here, but we can probably just use instance variable references for most of these. I doubt many are actually being accessed externally.
| prop :reflection, _Never | ||
| prop :parent_record, _Void | ||
| prop :parent_resource, _Void, reader: :public | ||
| prop :actions, _Void, reader: :public |
There was a problem hiding this comment.
Need to figure out what types these are. _Void here is always true, allows anything through.
There was a problem hiding this comment.
I’m pretty sure actions is _Nilable(_Array(Avo::BaseAction)) and reflection should be _Nilable(ActiveRecord::Reflections::AssociationReflection).
| prop :parent_component, _Void | ||
| prop :parent_record, _Nilable(ActiveRecord::Base) | ||
| prop :parent_resource, _Nilable(Avo::BaseResource) | ||
| prop :reflection, _Void |
There was a problem hiding this comment.
What type is a reflection?
| case @width | ||
| in :md | ||
| "w-11/12 lg:w-1/2 sm:max-w-168" | ||
| when :xl | ||
| in :xl |
There was a problem hiding this comment.
I switched this to case-in, since it’s more exhaustive. It has a default else-raise.
joeldrapper
force-pushed
the
literal-properties
branch
from
e30ff5c to
506cd3b
Compare
|
Hey @joeldrapper. Is this "ready" to review? Or should we have another look over it? |
|
I forgot that today is Brighton Ruby day... |
|
@adrianthedev I think this PR is too big. I’m going to open some smaller ones instead. |
Description
[WIP] This PR introduces Literal properties.
Note: this should follow #2892. I couldn’t stack it on that branch because the branch is on a fork.
Checklist: