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

Adding a welcoming page prompting to add a Provider #5434

Merged
merged 8 commits into from
May 2, 2019

Conversation

romanblanco
Copy link
Member

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1678190

Before:
Screenshot_2019-04-05 ManageIQ Infrastructure Providers(1)

After:
Screenshot_2019-04-05 ManageIQ Infrastructure Providers

Links

Steps for Testing/QA

  • Login with an empty database

@romanblanco
Copy link
Member Author

@miq-bot add_label ux/review
cc/ @terezanovotna

@Loicavenel
Copy link

@romanblanco @terezanovotna is it when I log for the first time to CF just after install? or then I got to Infra Provider for the first time?

@romanblanco
Copy link
Member Author

romanblanco commented Apr 5, 2019

@Loicavenel the new screen appears anytime there are no Infrastructure Providers
(if user removes all the Infrastructure Providers from database, the new screen is used)

@Loicavenel
Copy link

@romanblanco is it for every providers?

@romanblanco
Copy link
Member Author

@Loicavenel No. Just thought it probably should be for every. I'll update the PR.

@terezanovotna
Copy link

This looks awesome!!

  1. Where does the documentation take me?
  2. there was a comment in bz with updated text. pick one you prefer.

Title: Add a provider
Body: Welcome! To start using Red Hat Cloud Forms, you need to add a
provider. Learn more about adding and managing infrastructure, cloud, and
container providers in the documentation.

or more general

"Learn more about providers in the documentation."

  1. Can we have the empty state grey area full screen? So it spreads all the way to the bottom? @kybaker is that correct?

"data-miq_sparkle_on" => true,
"data-miq_sparkle_off" => true,
"data-method" => :post,
:title => _("Add a new %{provider_type} Provider") % {:provider_type => type})
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to avoid doing string arithmetics with gettext strings as much as possible (i.e. we should avoid string concatenations). In this case, may I suggest to add the full string from parent view:

= render :partial => 'layouts/empty', :locals => {:add_text => _("Add a new Network Provider")}

and then just render the text here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mzazrivec updated.

@romanblanco
Copy link
Member Author

romanblanco commented Apr 8, 2019

@terezanovotna to answer questions 1. and 2.

Where does the documentation take me?

since the last commit (0b6d7ad) the documentation links to different sections of http://manageiq.org/docs/reference/latest/doc-Managing_Providers/miq/, depending on the provider type:

there was a comment in bz with updated text. pick one you prefer.

I'd avoid using "Welcome! To start using Red Hat Cloud Forms..." as the page is used on more providers pages. The user might not be using the product for the first time. Also less problems with productization (ManageIQ vs. Red Hat CloudForms). I'll keep the general one.

@Loicavenel
Copy link

@romanblanco We are missing Ansible Tower.. URL is : https:///automation_manager/explorer#/

@romanblanco
Copy link
Member Author

@Loicavenel updated.

@terezanovotna
Copy link

the wording looks good 👍
If we can make it full-screen grey and center align the empty state, that would be even better.

Nice job!

@romanblanco
Copy link
Member Author

romanblanco commented Apr 15, 2019

update:

Screenshot_2019-04-15 ManageIQ Cloud Providers

to get the empty state full-screen gray, it seems that .container-fluid needs to have a CSS style miq-sand-paper applied.

The problem is, that now would apply to the toolbar buttons and breadcrumbs as well, which is not desired.

@skateman @epwinchell, would you know how this could be solved?

@romanblanco romanblanco changed the title Adding a welcoming page prompting to add a Provider [WIP] Adding a welcoming page prompting to add a Provider Apr 15, 2019
@miq-bot miq-bot added the wip label Apr 15, 2019
@epwinchell
Copy link
Contributor

@miq-bot add_reviewer @epwinchell

@miq-bot miq-bot requested a review from epwinchell April 15, 2019 13:33
@epwinchell
Copy link
Contributor

epwinchell commented Apr 15, 2019

@romanblanco I think the welcoming page should be using _center_div_no_listnav layout. We just merged a PR: ( https://github.com/ManageIQ/manageiq-ui-classic/pull/5455/files) that adds a method using "miq-body" (not miq-sand-paper, which is specific to GTL) for situations just like this.

cc @himdel

@himdel
Copy link
Contributor

himdel commented Apr 16, 2019

Agreed, instead of changing center_div_with_listnav not to have a listnav sometimes, it would be better to use a center_div_* variant without a listnav in the first place. We have 2, the dashboard variant sounds specific to dashboard, so center_div_no_listnav it is.

It could be a just about appending another return false if... to layout_uses_listnav?,
possibly something like @showtype == 'show_list' && controller.class.model.none?.
(Assuming consistently set @showtype, which is ..not entirely guaranteed :).)
(You may need to add a method to those controllers to specify the class of the thing, or nil from application controller, to have the layout_uses_listnav? logic kick in only where it should be used)

@romanblanco romanblanco changed the title [WIP] Adding a welcoming page prompting to add a Provider Adding a welcoming page prompting to add a Provider Apr 16, 2019
@miq-bot miq-bot removed the wip label Apr 16, 2019
@romanblanco
Copy link
Member Author

romanblanco commented Apr 16, 2019

@himdel

(You may need to add a method to those controllers to specify the class of the thing, or nil from application controller, to have the layout_uses_listnav? logic kick in only where it should be used)

not sure if I understand, but what I got now, seems to work correctly
EDIT: except the failing spec

@epwinchell @terezanovotna please review

@himdel
Copy link
Contributor

himdel commented Apr 16, 2019

not sure if I understand, but what I got now, seems to work correctly

👍 LGTM by me :)

What I meant is that you may need new controller method if class.model were not enough, or if you want to prevent having another whitelist there. But whitelist is fine too, and model works, so.. 👍

Copy link
Contributor

@epwinchell epwinchell left a comment

Choose a reason for hiding this comment

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

@romanblanco Tested. Looks great.

@himdel
Copy link
Contributor

himdel commented Apr 18, 2019

@romanblanco Still missing a couple:

Configuration > Management - Providers ... Foreman providers
Storage > Block Storage > Managers ... cinder
Storage > Object Storage > Managers ... swift

though the storage manager screens are special in that you can't create those directly, they come to being by adding an cloud/infra provider, so not sure about those..

Otherwise LGTM too, let me know if you want this merged now, and deal with the rest separately..

@Loicavenel
Copy link

@romanblanco For Storage, providers cannot be added there, they are coming when OpenStack or RHV is added. I think we should put just a message without Button...

@romanblanco romanblanco force-pushed the bz1678190 branch 2 times, most recently from f17d1bf to 5409aec Compare May 2, 2019 13:50
@miq-bot
Copy link
Member

miq-bot commented May 2, 2019

Checked commits romanblanco/manageiq-ui-classic@f65634a~...dc18540 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

app/helpers/application_helper/page_layouts.rb

@romanblanco
Copy link
Member Author

I believe the PR is ready.

All the show_list screens with an option to add a provider should work well, the landing page is fullscreen as requested.

In case of explorer screens, the landing page is placed next to the accordions, as the logic to make them fullscreen as in case of show_list would be too invasive.

Other changes can be done in a separate PR.

@himdel
Copy link
Contributor

himdel commented May 2, 2019

Makes sense, in explorers, having those other bits missing may break any ajax transitions, so that will be harder.

@miq-bot add_label changelog/yes
(I think :))

@himdel himdel self-assigned this May 2, 2019
@himdel himdel merged commit b8e7e2e into ManageIQ:master May 2, 2019
@himdel himdel added this to the Sprint 111 Ending May 13, 2019 milestone May 2, 2019
@skateman
Copy link
Member

skateman commented May 2, 2019

I feel this new page as very welcoming 🤣 sorry

@romanblanco romanblanco deleted the bz1678190 branch May 2, 2019 18:23
@himdel
Copy link
Contributor

himdel commented Jun 21, 2019

@romanblanco One issue - when this happens in an explorer screen, you can still click on the tree root (to get to the "same screen"), and get to the old "No records found" screen...

Have no foreman providers,
Go to Configuration > Management ,
you'll see the new "Add a Provider" screen.
Click on "All Configuration Manager Providers" in the tree,
bad

(This may not really be fixable in explorers, I'm not sure, we don't really do ajax transitions between different layouts so.. just putting it here.)

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