-
Notifications
You must be signed in to change notification settings - Fork 125
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 columns enablement_expression and disabled_text to CustomButtons #29
Add columns enablement_expression and disabled_text to CustomButtons #29
Conversation
@lpichler Cannot apply the following label because they are not recognized: gaprindashvili |
As in it will contain a serialized MiqExpression? |
class AddMiqExpressionEnableAndTextDisable < ActiveRecord::Migration[5.0] | ||
def change | ||
add_column :custom_buttons, :miq_expression_enable, :text | ||
add_column :custom_buttons, :text_disable, :text |
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.
I'm thinking disabled_text
sounds a little better. Otherwise it reads to me like "whether or not to disable the text portion of the button".
@@ -0,0 +1,6 @@ | |||
class AddMiqExpressionEnableAndTextDisable < ActiveRecord::Migration[5.0] | |||
def change | |||
add_column :custom_buttons, :miq_expression_enable, :text |
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.
I'm thinking enablement_expression
or enablement_miq_expression
. The word enable is a verb, so it feels weird to me when it's used as a noun/adjective.
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.
We've been quite inconsistent with naming (thinking condition
, filter
, expression
), but I think enablement_<one of those three>
sounds good to me
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.
Cool, now to decide which one of the 3 😆 ... I'm fine with just _expression
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.
That sounds good to me!
@imtayadeway Thoughts on my comments above? |
miq_expression_enable - it is for MiqExpression, which after evaluation will say whether the button will be enabled or disabled text_disabled - is for text which will be displayed when the button will be disabled.
7de8953
to
5b31b3b
Compare
Checked commit lpichler@5b31b3b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
used |
@Fryguy yes, e.g.(like MiqReport#conditions):
and serialization is added here ManageIQ/manageiq#15502 |
enablement_expression
- it is for MiqExpression, which afterevaluation will say whether the button will be enabled or disabled
disabled_text
- is for text which will be displayed when thebutton will be disabled.
Links
https://www.pivotaltracker.com/n/projects/1608513/stories/147780727
Serialization is added here ManageIQ/manageiq#15502
@miq-bot add_label gaprindashvili
cc @gtanzillo
@miq-bot assign @Fryguy