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

Added custom button menu to Resource Details page #1254

Merged
merged 7 commits into from
Dec 8, 2017
Merged

Added custom button menu to Resource Details page #1254

merged 7 commits into from
Dec 8, 2017

Conversation

chalettu
Copy link
Contributor

@miq-bot add_label gaprindashvili/yes
@miq-bot add_label enhancement
BZ - https://bugzilla.redhat.com/show_bug.cgi?id=1513113

@chalettu
Copy link
Contributor Author

Before SS
before_ss

@chalettu
Copy link
Contributor Author

After SS
new_menu

@Loicavenel
Copy link

@AllenBW I am confused here. the design if I am not mistaking is to have the kebab menu when there are too many Custom button on the top bar not all the time...

@Loicavenel
Copy link

@serenamarie125 need your comment here..

@chalettu
Copy link
Contributor Author

chalettu commented Nov 17, 2017

@Loicavenel , no need to ask @AllenBW , I was the one working on this issue. This kebab menu will not show all the time, we put a limit of 3 custom buttons or button groups to show on the screen before we show the kebab menu instead. This limit can be programmatically adjusted we just wanted to start with an implementation that would prevent button overflow on the screen.

@Loicavenel
Copy link

@chalettu my apologise..

@chalettu
Copy link
Contributor Author

No worries @Loicavenel . Screenshots and small videos can sometimes make it hard to convey other logic behind what is intended.

@chriskacerguis
Copy link
Contributor

chriskacerguis commented Nov 17, 2017

@chalettu could you please address the CC issues?

@Loicavenel I think what @chalettu is showing is that it is limiting the buttons now (it's not all the time)...at least that is not what the desired behavior is. @chalettu could you clarify?

Copy link

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

@chalettu spacing between buttons is an issue. I think I have another Issue about this, but can't find it now.
Questions:

  • what is "SS" ??? The custom button group should be shown on the toolbar in this case, I'm a bit confused
  • why is there a tooltip on each of the menu items?

@chalettu
Copy link
Contributor Author

@serenamarie125 , I agree that button spacing is an issue. Chris K had created another issue to address it for clarity sake and being consistent.
SS is just an abbreviation for screen shot. So the screenshots are for demo purposes. The code as implemented will only default to kebab menu after 3 buttons/button groups are defined. We can change this count if you like.
The tooltips are aren't necessary and we can remove them if you feel they don't fit for that drop down. Please let me know and we can definitely have them removed.

@serenamarie125
Copy link

LOL - i'm being too literal sorry @chalettu now i get the "SS"

tooltips are fine, @Loicavenel just showed me that you can set hover text, so that makes sense.

I do see an issue that the drop down menu has a caret at the top ... it should be straight, similar to the user menu shown below
image

@chalettu
Copy link
Contributor Author

@serenamarie125 , Here is a SS with the caret removed. Please let me know if we think any more changes might be required.
testing

@serenamarie125
Copy link

@chalettu this is really more complex :\

Kebab uses caret (http://www.patternfly.org/pattern-library/widgets/#kebabs)

  • note that the kebab has a selected state and the caret should be centered - this screenshot is from the code example from the link above

screen shot 2017-11-20 at 8 13 23 pm

The regular menus ( snapshots & power operations) shouldn't

@AllenBW
Copy link
Member

AllenBW commented Dec 4, 2017

@chalettu Any update on centering of the kebab dropdown ?

@AllenBW AllenBW assigned ohadlevy and unassigned chriskacerguis Dec 4, 2017
@AllenBW AllenBW assigned AllenBW and unassigned ohadlevy Dec 4, 2017
@chalettu
Copy link
Contributor Author

chalettu commented Dec 4, 2017

@AllenBW , haven't come back to this one. I will try and take a look at centering the kebab.

@AllenBW
Copy link
Member

AllenBW commented Dec 4, 2017

@chalettu I suspect this might be a less is more kinda situation... rip out classes see what changes... 🤔

@chalettu chalettu requested a review from himdel as a code owner December 6, 2017 19:32
@chalettu
Copy link
Contributor Author

chalettu commented Dec 6, 2017

Updated SS where I added back the caret and centered it.
updated_caret

@chalettu
Copy link
Contributor Author

chalettu commented Dec 6, 2017

@serenamarie125 , can you take a look at this latest screenshot and see if it matches your expectations of this caret menu?

Copy link

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

perfect @chalettu thank you!

@serenamarie125
Copy link

@miq-bot add_labels ux/approved

@serenamarie125
Copy link

@miq-bot remove_labels ux/review

@serenamarie125
Copy link

@miq-bot add_label ux/approved

@serenamarie125
Copy link

@miq-bot remove_label ux/review

@AllenBW AllenBW added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 7, 2017
Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

@chalettu ok just the one thing, ONE THING then this is g2g 🙇 ❤️

}
}

function postFailure () {
Copy link
Member

Choose a reason for hiding this comment

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

There's gonna be real valuable server feedback on the error here, something similar to this should be used here

@miq-bot
Copy link
Member

miq-bot commented Dec 7, 2017

Checked commits https://github.com/chalettu/manageiq-ui-service/compare/7f09ef9d7c5909f5ec50565db7c14b84b57362a9~...24ad37e28fdda13f3dc3b30efb81ad1336742441 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 👍

@chalettu
Copy link
Contributor Author

chalettu commented Dec 7, 2017

@AllenBW , I updated based on feedback. Let me know if you approve.

Copy link
Member

@AllenBW AllenBW left a comment

Choose a reason for hiding this comment

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

Looks delicious 😋 thanks for updating!!

@AllenBW AllenBW merged commit 672d128 into ManageIQ:master Dec 8, 2017
@AllenBW AllenBW deleted the custom-button-grouping branch December 8, 2017 12:10
simaishi pushed a commit that referenced this pull request Dec 11, 2017
Added custom button menu to Resource Details page
(cherry picked from commit 672d128)

https://bugzilla.redhat.com/show_bug.cgi?id=1524718
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 0b8e5730075e7feead9d824be159191f2124679a
Author: Allen Wight <[email protected]>
Date:   Fri Dec 8 07:10:47 2017 -0500

    Merge pull request #1254 from chalettu/custom-button-grouping
    
    Added custom button menu to Resource Details page
    (cherry picked from commit 672d1281341c8e7f818d1779682679e9727abfa4)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1524718

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