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 #16379 - Improve layout of hostname randomize button #3792

Closed
wants to merge 1 commit into from

Conversation

tbrisker
Copy link
Member

This moves the button from being a line of its own to an icon inside the
input. Also added a form_feedback option to the field helper to allow
easy addition of more icons in inputs in the future.

@tbrisker
Copy link
Member Author

Before:
screenshot from 2016-08-30 11-02-11

After:
screenshot from 2016-08-30 11-01-29

@Rohoover - I'd be happy for your input

@ares
Copy link
Member

ares commented Aug 30, 2016

I think it deserves its own redmine issue.

@tbrisker tbrisker changed the title Refs #13768 - Improve layout of hostname randomize button Fixes #16379 - Improve layout of hostname randomize button Aug 30, 2016
@tbrisker
Copy link
Member Author

@ares thanks, updated

@ares
Copy link
Member

ares commented Aug 30, 2016

Works nicely, ACK pending jenkins and feedback from @Rohoover

@ohadlevy
Copy link
Member

@tbrisker how about having it as a btn inside the input, similar to the date picker at https://www.patternfly.org/pattern-library/forms-and-controls/date-picker ?

@tbrisker
Copy link
Member Author

@ohadlevy I personally think it looks nicer not as a button, but I may be wrong. I'll do whatever @Rohoover says will work better.

@Rohoover
Copy link

Rohoover commented Aug 31, 2016

Visually I am not bothered by either approach so I have to default to the closest standard for consistency which would be what @ohadlevy suggested.

Leaving it "naked" without the button would be a new pattern or exception.

@@ -331,14 +320,15 @@ def field(f, attr, options = {})
yield
end.html_safe
else
form_feedback = content_tag(:span, options.delete(:form_feedback), :class => "form-control-feedback")

Choose a reason for hiding this comment

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

Useless assignment to variable - form_feedback.

This moves the button from being a line of its own to a button in the
input.
Also changed the form helper to streamline adding button to inputs and
got rid of a useless helper.
@tbrisker
Copy link
Member Author

Changed to a button, updated screenshot:
screenshot from 2016-08-31 15-46-20

@ohadlevy
Copy link
Member

@tbrisker what do you see if the feature is disabled?

@tbrisker
Copy link
Member Author

@ohadlevy just a regular text input with no button

@ohadlevy
Copy link
Member

ack assuming this is the right icon to use (didnt find a better one).

@Rohoover
Copy link

This is the correct icon. It's labeled as such in font awesome and is the generally accepted icon for "random" in a multitude of applications. +1

@lzap
Copy link
Member

lzap commented Aug 31, 2016

Works as expected, thanks.

@ehelms
Copy link
Member

ehelms commented Sep 1, 2016

[test]

@lzap
Copy link
Member

lzap commented Sep 1, 2016

Merged as 3b6f932, thank you!

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