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

Split Chargeback explorer into 3 explorers #7016

Merged
merged 3 commits into from
May 12, 2020

Conversation

h-kataria
Copy link
Contributor

@h-kataria h-kataria commented May 1, 2020

Removed Accordions from Chargeback Explorer, splitting that into 3 individual explorers with their third level subtabs under Overview/Chargeback, third level tabs are Reports/Rates/Assignments. These new subtabs still use old Product features "chargeback_explorer" that will be taken care of when explorers are converted into non-explorer style screens as this will also require migration at the time.
Made changes to remove functionality from new screens to show collapsible accordion on the top like other explorer style screen
Split spec tests into multiple files based upon new controller/explorer structure

Partial fix for #6996

Core PR affected by changes in this PR ManageIQ/manageiq#20129

Before
chargeback_explorer

After
cb_reports

cb_rates

cb_assignments

@h-kataria h-kataria added the redesign Screen Redesign label May 1, 2020
@h-kataria h-kataria force-pushed the remove_chargeback_accordions branch 3 times, most recently from 2a58ae7 to c1b0398 Compare May 1, 2020 21:56
@h-kataria h-kataria removed the wip label May 6, 2020
@h-kataria h-kataria changed the title [WIP] Split Chargeback explorer into 3 explorers Split Chargeback explorer into 3 explorers May 6, 2020
@h-kataria h-kataria requested a review from himdel May 6, 2020 15:33
h-kataria added 2 commits May 8, 2020 18:39
Removed Accordions from Chargeback Explorer, splitting that into 3 individual explorers with their third level subtabs under Overview/Chargeback, third level tabs are Reports/Rates/Assignments. These new subtabs still use old Product features "chargeback_explorer" that will be taken care of when explorers are converted into non-explorer style screens as this will also require migration at the time.
Made changes to remove functionality from new screens to show collapsible accordion on the top like other explorer style screen
Split spec tests into multiple files based upon new controller/explorer structure

Partially fixes ManageIQ#6996
This commit will allow direct url with id of rate to work in chargeback rates explorer
- http://localhost:3000/chargeback_rate/explorer/ (loads Rates explorer)
- http://localhost:3000/chargeback_rate/x_show/9  (loads summary screen of rate using id passed in the url)

Partially fixes ManageIQ#6996
@h-kataria h-kataria force-pushed the remove_chargeback_accordions branch from bcc2a80 to a16cf8f Compare May 8, 2020 22:40
h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request May 9, 2020
- Added a button in GTL view toolbar to add ability to toggle between list view and tree view. Tree has links to Chargeback Rate records, User can click on "Rates" link in breadcrumbs to go back to list view.
- Updated all toolbar buttons to work in non-explorer modes

Follow up PR for ManageIQ#7016

Fixes ManageIQ#6996
h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request May 10, 2020
- Added a button in GTL view toolbar to add ability to toggle between list view and tree view. Tree has links to Chargeback Rate records, User can click on "Rates" link in breadcrumbs to go back to list view.
- Updated all toolbar buttons to work in non-explorer modes

Follow up PR for ManageIQ#7016

Fixes ManageIQ#6996
@skateman skateman self-requested a review May 11, 2020 15:19
@Fryguy
Copy link
Member

Fryguy commented May 11, 2020

Can we use maybe fa-sitemap for the tree icon? https://fontawesome.com/icons?d=gallery&q=sitemap

I would prefer if the tree icon was visually left-right oriented (as opposed to top-down oriented like sitemap), but I couldn't find an icon like that.

@skateman
Copy link
Member

@Fryguy yeah, but we're on FA4, so https://fontawesome.com/v4.7.0/icon/sitemap instead

@h-kataria
Copy link
Contributor Author

@Fryguy @skateman using fa fa-sitemap
Screenshot from 2020-05-11 13-14-00

@Fryguy
Copy link
Member

Fryguy commented May 11, 2020

I think that's good for now

h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request May 11, 2020
- Added a button in GTL view toolbar to add ability to toggle between list view and tree view. Tree has links to Chargeback Rate records, User can click on "Rates" link in breadcrumbs to go back to list view.
- Updated all toolbar buttons to work in non-explorer modes

Follow up PR for ManageIQ#7016

Fixes ManageIQ#6996
h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request May 11, 2020
- Added a button in GTL view toolbar to add ability to toggle between list view and tree view. Tree has links to Chargeback Rate records, User can click on "Rates" link in breadcrumbs to go back to list view.
- Updated all toolbar buttons to work in non-explorer modes

Follow up PR for ManageIQ#7016

Fixes ManageIQ#6996
h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request May 11, 2020
- Added a button in GTL view toolbar to add ability to toggle between list view and tree view. Tree has links to Chargeback Rate records, User can click on "Rates" link in breadcrumbs to go back to list view.
- Updated all toolbar buttons to work in non-explorer modes

Follow up PR for ManageIQ#7016

Fixes ManageIQ#6996
h-kataria added a commit to h-kataria/manageiq that referenced this pull request May 12, 2020
Added and reorganized features under Chargeback Rates node to support changes in non-explorer version of Chargeback Rates screen.

Follow up PR for ManageIQ/manageiq-ui-classic#7016

Fixes ManageIQ#6996
h-kataria added a commit to h-kataria/manageiq that referenced this pull request May 12, 2020
Added and reorganized features under Chargeback Rates node to support changes in non-explorer version of Chargeback Rates screen.

Follow up PR for ManageIQ/manageiq-ui-classic#7016

Fixes ManageIQ#6996
@@ -216,20 +216,32 @@ def valid_html_id(id)
id
end

NO_ACCORDION = %w[chargeback_assignment
Copy link
Member

Choose a reason for hiding this comment

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

This is definitely not a good solution, let me think about it a little.

@skateman
Copy link
Member

Regarding the accordions: for consistency I think we should still display the page with a single one, like we do it on other pages:
Screenshot from 2020-05-12 15-52-33

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

as described in the comments above

- Added miq_accordion styling back in chargeback explorer screens to be consistent with other explorers with only one accordion.
- Removed redundant `tree_autoload` route from chargeback_rates controller

Partially fixes ManageIQ#6996
@miq-bot
Copy link
Member

miq-bot commented May 12, 2020

Some comments on commits h-kataria/manageiq-ui-classic@21918d8~...2b632f5

spec/controllers/chargeback_rate_controller_spec.rb

  • ⚠️ - 72 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

Sorry, something went wrong.

@miq-bot
Copy link
Member

miq-bot commented May 12, 2020

Checked commits h-kataria/manageiq-ui-classic@21918d8~...2b632f5 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
32 files checked, 35 offenses detected

app/controllers/chargeback_assignment_controller.rb

app/controllers/chargeback_rate_controller.rb

app/controllers/chargeback_report_controller.rb

app/helpers/application_helper/toolbar_chooser.rb

app/presenters/menu/default_menu.rb

app/views/chargeback_assignment/explorer.html.haml

  • ⚠️ - Line 2 - Layout/TrailingBlankLines: Final newline missing.
  • ⚠️ - Line 2 - id attribute must be in lisp-case

app/views/chargeback_rate/explorer.html.haml

  • ⚠️ - Line 2 - Layout/TrailingBlankLines: Final newline missing.
  • ⚠️ - Line 2 - id attribute must be in lisp-case

app/views/chargeback_rate/x_show.html.haml

  • ⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.

app/views/chargeback_report/explorer.html.haml

  • ⚠️ - Line 2 - Layout/TrailingBlankLines: Final newline missing.
  • ⚠️ - Line 2 - id attribute must be in lisp-case

config/routes.rb

spec/controllers/chargeback_assignment_controller_spec.rb

spec/controllers/chargeback_report_controller_spec.rb

@skateman
Copy link
Member

Can you show me an example URL that should redirect me to a location deep in the hierarchy?

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

The Seal of Approval

@skateman skateman merged commit 22c37ca into ManageIQ:master May 12, 2020
h-kataria added a commit to h-kataria/manageiq that referenced this pull request May 12, 2020
Added and reorganized features under Chargeback Rates node to support changes in non-explorer version of Chargeback Rates screen.

Follow up PR for ManageIQ/manageiq-ui-classic#7016

Fixes ManageIQ#6996
h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request May 12, 2020
- Added a button in GTL view toolbar to add ability to toggle between list view and tree view. Tree has links to Chargeback Rate records, User can click on "Rates" link in breadcrumbs to go back to list view.
- Updated all toolbar buttons to work in non-explorer modes

Follow up PR for ManageIQ#7016

Fixes ManageIQ#6996
h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request May 12, 2020
- Added a button in GTL view toolbar to add ability to toggle between list view and tree view. Tree has links to Chargeback Rate records, User can click on "Rates" link in breadcrumbs to go back to list view.
- Updated all toolbar buttons to work in non-explorer modes

Follow up PR for ManageIQ#7016

Fixes ManageIQ#6996
h-kataria added a commit to h-kataria/manageiq-schema that referenced this pull request May 13, 2020
This migration will update startpage entry for any existing user from `chargeback/explorer` to `chargeback_reports/explorer`

This issue was caused by changes in ManageIQ/manageiq-ui-classic#7016
h-kataria added a commit to h-kataria/manageiq-schema that referenced this pull request May 13, 2020
This migration will update startpage entry for any existing user from `chargeback/explorer` to `chargeback_reports/explorer`

This issue was caused by changes in ManageIQ/manageiq-ui-classic#7016
@h-kataria h-kataria deleted the remove_chargeback_accordions branch May 13, 2020 17:58
h-kataria added a commit to h-kataria/manageiq-schema that referenced this pull request May 18, 2020
This migration will update startpage entry for any existing user from `chargeback/explorer` to `chargeback_reports/explorer`

This issue was caused by changes in ManageIQ/manageiq-ui-classic#7016
simaishi pushed a commit that referenced this pull request May 18, 2020
Split Chargeback explorer into 3 explorers

(cherry picked from commit 22c37ca)
@simaishi
Copy link
Contributor

Jansa backport details:

$ git log -1
commit 5b6a5526bc8838e10ff0560788b969054571c1d2
Author: Halász Dávid <[email protected]>
Date:   Tue May 12 17:38:09 2020 +0200

    Merge pull request #7016 from h-kataria/remove_chargeback_accordions

    Split Chargeback explorer into 3 explorers

    (cherry picked from commit 22c37cab86e246c369858c5aca7dfb8aa3203ed2)

h-kataria added a commit to h-kataria/manageiq-schema that referenced this pull request May 18, 2020
This migration will update startpage entry for any existing user from `chargeback/explorer` to `chargeback_reports/explorer`

This issue was caused by changes in ManageIQ/manageiq-ui-classic#7016
kbrock pushed a commit to kbrock/manageiq-ui-classic that referenced this pull request Aug 8, 2020
- Added a button in GTL view toolbar to add ability to toggle between list view and tree view. Tree has links to Chargeback Rate records, User can click on "Rates" link in breadcrumbs to go back to list view.
- Updated all toolbar buttons to work in non-explorer modes

Follow up PR for ManageIQ#7016

Fixes ManageIQ#6996
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.

None yet

7 participants