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

Toolbars - use send_checked, refactor params in miqToolbarOnClick #2398

Merged
merged 6 commits into from Nov 14, 2017
Merged

Toolbars - use send_checked, refactor params in miqToolbarOnClick #2398

merged 6 commits into from Nov 14, 2017

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Oct 13, 2017 •
edited

Depends on ManageIQ/ui-components#185

Fixes #588 (except for cleanup)

  • makes send_checked propagate from toolbar definitions to the toolbar component
  • makes custom buttons send send_checked: true whenever id=LIST
  • refactors miqToolbarOnClick and changes to use send_checked instead of url_params when determining when to send miq_grid_checks:

miqToolbarOnClick differences

  • we no longer use url_parms to know when to send miq_grid_checks

    • before: when ending with _div and nonempty, or containing id=LIST
    • now: when send_checked and nonempty
    • change: params[:miq_grid_checks] will be nil instead of "" when nothing is selected even for custom buttons

  • we always call miqSerializeForm when ending with _div, but clean up nothings

    • before: when _div and (gridChecks) empty
    • now: always, but returns "" for GTLs
    • (but the alternative of sending when _div and not send_checked or empty is probably wrong anyway)

  • the ? url_params variant is handled a bit differently

    • ? position
      • before: this would affect any url_parms containing a question mark
      • now: url_parms needs to start with it
      • but not seeing any url_parms values where this would matter
    • multiple ?
      • before: split[1] pretty much means just anything between the first two question marks
      • now: slice(1) will use the whole string
      • but not seeing any url_parms values where this would matter

Cc @martinpovolny

@himdel himdel mentioned this pull request Oct 13, 2017
Closed
@himdel himdel mentioned this pull request Oct 23, 2017
Closed
@himdel himdel closed this Oct 23, 2017
@himdel himdel reopened this Oct 23, 2017
@martinpovolny
Copy link
Member

Travis restarted.

@himdel himdel closed this Oct 31, 2017
@himdel himdel reopened this Oct 31, 2017
@himdel
Copy link
Contributor Author

himdel commented Nov 2, 2017

Fixed specs (and a bug in JS when url_parms was undefined) :)

@martinpovolny
Copy link
Member

@himdel: all the CC issues are easily fixable ;-)

@himdel
Copy link
Contributor Author

himdel commented Nov 7, 2017 •
edited

@martinpovolny Yes but do we actually want them fixed?

Identifier 'url_parms' is not in camel case. - easy fix, but then searching for url_parms won't find the code. (In general I'm starting to think the camelCase rule doesn't make sense for any code interacting with the ruby bits.)

'params' is already declared in the upper scope. - sure it is. Which made me check that this is really supposed to be a different params, it is, mission accomplished :). I can of course rename the new one to outputParams or whatever, but.. is there any point? :)

WDYT? :)
I'll of course fix both if you want me to, just saying, both are marked as warnings and that's on purpose.
If we really want to start enforcing rules 100% time, we need to drop a few.

(That said, 6d6755f should fix those warnings.)

@miq-bot
Copy link
Member

miq-bot commented Nov 13, 2017

This pull request is not mergeable. Please rebase and repush.

@martinpovolny
Copy link
Member

'params' is already declared in the upper scope. - sure it is. Which made me check that this is really supposed to be a different params, it is, mission accomplished :). I can of course rename the new one to outputParams or whatever, but.. is there any point? :)

Please, fix this one. It's a readability issue.

Also you are setting the standards here. So please, set it high ;-)

@himdel
miqToolbarOnClick - extract params handling into a separate function
4a6aee3 
hopefully cleaner, more obvious that it does two kinds of two kinds of things :)
there should be no functional change (thus that explicit return undefined)
@himdel
ToolbarBuilder#create_custom_button - add send_checked when id=LIST
6cf594c 
so that we no longer need to special-case id=LIST in miqToolbarOnClick

(the magic LIST value remains used `ApplicationController::Buttons#custom_buttons`)
@himdel
ToolbarBuilder#apply_common_props - propagate send_checked
2fb4a83
@himdel
miqToolbarOnClick>getParams - use send_checked, simplify
df969ee 
  * we no longer use `url_parms` to know when to send `miq_grid_checks`
    * before: when ending with `_div` and nonempty, or containing `id=LIST`
    * now: when `send_checked` and nonempty
    * change: `params[:miq_grid_checks]` will be `nil` instead of `""` when nothing is selected

  * we always call `miqSerializeForm` when ending with `_div`, but clean up nothings
    * before: when `_div` and empty
    * now: always, but returns `""` for GTLs
    * (but the alternative of sending when _div and not send_checked or empty is probably wrong anyway)

  * the ? url_params variant is handled a bit differently
    * `?` position
      * before: this would affect any url_parms containing a question mark
      * now: url_parms needs to start with it
      * but not seeing any url_parms values where this would matter
    * multiple `?`
      * before: split[1] pretty much means just anything between the first two question marks
      * now: slice(1) will use the whole string
      * but not seeing any url_parms values where this would matter
@himdel
ToolbarBuilder spec - add missing send_checked to download_pdf
8e26027
@himdel
toolbarOnClick - fix eslint warnings
b7f8ff4
@miq-bot
Copy link
Member

miq-bot commented Nov 14, 2017

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/4a6aee37f8c343147eb26c66671c309fa1594db8~...b7f8ff4a3994147c96baa98a0c5b9e3defe17d39 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@himdel
Copy link
Contributor Author

himdel commented Nov 14, 2017

Ok, ok, updated to address CC warnings .. aand rebased :).

@martinpovolny martinpovolny merged commit d0f08da into ManageIQ:master Nov 14, 2017
@martinpovolny martinpovolny added this to the Sprint 74 Ending Nov 27, 2017 milestone Nov 14, 2017
@martinpovolny martinpovolny self-assigned this Nov 14, 2017
@himdel himdel deleted the toolbaronclick branch November 14, 2017 16:55
@AparnaKarve AparnaKarve mentioned this pull request Dec 15, 2017
Merged
@himdel himdel mentioned this pull request Jan 4, 2018
Merged
@himdel himdel mentioned this pull request Jan 18, 2018
Merged
@himdel himdel mentioned this pull request Nov 6, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinpovolny martinpovolny Awaiting requested review from martinpovolny

Assignees

@martinpovolny martinpovolny

Labels
technical debt toolbars
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@himdel @martinpovolny @miq-bot