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

Fixes #17966 - Change buttons to primary for top level pages #6528

Merged
merged 1 commit into from Jan 6, 2017

Conversation

Projects
None yet
5 participants
@johnpmitsch
Copy link
Member

johnpmitsch commented Jan 6, 2017

No description provided.

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Jan 6, 2017

@johnpmitsch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @waldenraines, @ehelms and @cfouant to be potential reviewers.

@waldenraines

This comment has been minimized.

Copy link
Member

waldenraines commented Jan 6, 2017

I replaced all the .btn-primary in "New _______" buttons with .btn-default specifically because it's only a primary action when you're first setting up or when you are developing katello. I wouldn't imagine our users are constantly creating new things.

Maybe @Rohoover has thoughts on this? Maybe there is some guidance from patternfly on button colors?

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

johnpmitsch commented Jan 6, 2017

@waldenraines The guidelines that were mentioned on the foreman-dev mailing list ("Foreman Button Proposals") were:

Each page should have one clear call to action, where the primary action button is always blue, but all others are the secondary buttons(gray). There is an accessibility reason being that on enter the primary button should be enacted. It's possible that there could be no primary action button on a page.

I would think the "New ___ " button would be the primary action of our top level pages, I'm not sure how often a user actually uses the primary button factors in here, but curious to hear @Rohoover's thoughts

Looking at a repo page in Github, I see "Clone or Download" is made the primary action (in github's case its green) and that is not my usual reason for vising a repo page.

If we decide we need to make this change, I'll make it for all "New __ " buttons where appropiate

@waldenraines

This comment has been minimized.

Copy link
Member

waldenraines commented Jan 6, 2017

Looking at a repo page in Github, I see "Clone or Download" is made the primary action (in github's case its green) and that is not my usual reason for vising a repo page.

If we decide we need to make this change, I'll make it for all "New __ " buttons where appropiate

Good point and thanks for digging up the guidelines. I'm find changing them back but as you say above we would need to do so for all pages I have converted to non-nutupane.

I think part of the reason for me changing them was that in Nutupane we had multiple levels of primary action buttons which was confusing. Now that we only have one level it will probably look okay.

@Rohoover

This comment has been minimized.

Copy link

Rohoover commented Jan 6, 2017

@johnpmitsch @waldenraines

It's possible that something other than "New _____" would be the primary action depending on what a users primary task may be.

Based on the screen John sent me, your implementation of a primary button creating a New Product looks correct.

It should draw the users eye to the top level action on the page. And the patternfly primary blue is the correct color.

@waldenraines
Copy link
Member

waldenraines left a comment

Just marking this as request changes so we can hit all of the "New ________" buttons across the app.

Here's a list:

https://github.com/Katello/katello/pulls?utf8=%E2%9C%93&q=is%3Apr%20is%3Aclosed%20author%3Awaldenraines%20%22remove%20nutupane%20from%22

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

johnpmitsch commented Jan 6, 2017

@waldenraines @Rohoover thanks all, I'll go through the list and make the change where necessary (double checking there is only one primary action per page)

@johnpmitsch johnpmitsch force-pushed the johnpmitsch:new_product_button branch to e6f80fb Jan 6, 2017

@johnpmitsch

This comment has been minimized.

Copy link
Member Author

johnpmitsch commented Jan 6, 2017

@waldenraines updated

@waldenraines

This comment has been minimized.

Copy link
Member

waldenraines commented Jan 6, 2017

@johnpmitsch looks good. I will go back and update my open PRs to use .btn-primary as well.

@johnpmitsch johnpmitsch changed the title Fixes #17966 - Change New Product button to primary Fixes #17966 - Change buttons to primary for top level pages Jan 6, 2017

@johnpmitsch johnpmitsch merged commit 0e8b94e into Katello:master Jan 6, 2017

2 checks passed

default Job result: SUCCESS
Details
hound No violations found. Woof!

@johnpmitsch johnpmitsch deleted the johnpmitsch:new_product_button branch Jan 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.