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.

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

Cascading Auto Refresh for dialog fields #264

Merged
merged 10 commits into from
Feb 6, 2017

Conversation

eclarizio
Copy link
Member

@eclarizio eclarizio commented Jan 27, 2017

This changes the functionality of auto-refreshing dialog fields a little bit. Here's how they worked before:
If multiple fields were set to 'auto-refresh', any time another field that was set to 'trigger auto-refresh', all of those fields would refresh at the same time, and that would be the end of the chain. Sometimes, people were relying on one field being refreshed before the next, and this approach wouldn't work.

After this change, the triggers are processed from top to bottom, and any field that is set to 'auto-refresh' will auto-refresh, and then trigger the next field (if it's set to trigger) and so-on, until the end. of the tab. Tabs will no longer trigger auto-refreshes between each other like they used to.

https://www.pivotaltracker.com/story/show/138457551

/cc @gmcculloug

You can use this
dialog_export_20170126.yml.zip (I had to zip it up cause github won't allow yml files) for a sample dialog I was testing around with that includes all of the different types of dialog fields we have and some that depend on others.

You can use this datastore_2017_01_26.zip to get the automate methods that I used in the above dialog. The only relevant methods are in the "CascadingAutoRefresh" domain. Ignore the other domains, they're just what I had in my db.

@eclarizio
Copy link
Member Author

Tabs will no longer trigger auto-refreshes between each other like they used to.

Small correction with the latest update, this is no longer true and tabs will continue to be affected by auto-refreshes on other tabs, processing from the leftmost tab to the rightmost tab.

@gmcculloug
Copy link
Member

@syncrou Please review. @d-m-u Can review/test out this PR?

@d-m-u
Copy link
Contributor

d-m-u commented Feb 3, 2017

👍 Wow, this is really cool!! Looks all good to me.

@syncrou
Copy link
Contributor

syncrou commented Feb 3, 2017

@eclarizio - I like it. 👍

@gmcculloug
Copy link
Member

@eclarizio Overall this is looking good. The one thing I am not seeing at the moment is an auto-refresh triggered from a text field. I modified each text field on the second tab and no automate methods were run. Three of the text fields are configured to auto-refresh other fields.

The other fields seem to be working as expected.

var thisIsTheFieldToUpdate = function(event) {
tabIndex = event.data.tabIndex;
groupIndex = event.data.groupIndex;
fieldIndex = event.data.fieldIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Need the var declaration for tabIndex, groupIndex and fieldIndex

@@ -135,6 +143,7 @@ var dialogFieldRefresh = {
$('.dynamic-text-box-' + fieldId).val(responseData.values.text);
dialogFieldRefresh.setReadOnly($('.dynamic-text-box-' + fieldId), responseData.values.read_only);
dialogFieldRefresh.setVisible($('#field_' +fieldId + '_tr'), responseData.values.visible);
callback.call();
};

Copy link
Contributor

Choose a reason for hiding this comment

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

If easily doable, can you DRY up refreshTextAreaBox and refreshTextBox since both functions are so identical?

https://www.pivotaltracker.com/story/show/138457551

This ensures that the triggering of auto-refreshes for text boxes and
other fields that are relying on the observe javascript have the
current_index to compare against to figure out which field is next to
refresh.
@eclarizio
Copy link
Member Author

@gmcculloug Oops, should be fixed now, text boxes were behaving slightly differently due to how the observe functionality works.

@AparnaKarve Good catch on the var declaration. I ideally would've liked to refactor all of those 'done' functions since they're very similar, but you're right in that the text ones were identical aside from the selector so I've pulled that out. Thanks.

@miq-bot
Copy link
Member

miq-bot commented Feb 3, 2017

Checked commits eclarizio/manageiq-ui-classic@72a728f~...e68866d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 4 offenses detected

app/helpers/application_helper/dialogs.rb

app/views/shared/dialogs/_dialog_field.html.haml

  • ⚠️ - Line 16 - Avoid the use of double negation (!!).

spec/helpers/application_helper/dialogs_spec.rb

  • ❗ - Line 493, Col 9 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
  • ❗ - Line 498, Col 7 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.

Copy link
Contributor

@AparnaKarve AparnaKarve left a comment

Choose a reason for hiding this comment

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

Thanks @eclarizio! Looks good now.

@gmcculloug
Copy link
Member

Retested and the text fields are working now, thanks. Looks good.

@simaishi
Copy link
Contributor

simaishi commented Feb 9, 2017

Backported to Darga via ManageIQ/manageiq#13778

@simaishi
Copy link
Contributor

Backported to Euwe via ManageIQ/manageiq#13779

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

Successfully merging this pull request may close these issues.

8 participants