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

Removed Optional. label from 'Add New Container Provider' screen #3785

Merged
merged 1 commit into from
Aug 12, 2015

Conversation

romanblanco
Copy link
Member

@miq-bot
Copy link
Member

miq-bot commented Aug 10, 2015

Checked commit romanblanco@a1aa4c9 with rubocop 0.32.1 and haml-lint 0.13.0
0 files checked, 0 offenses detected
Everything looks good. 👍

@chessbyte
Copy link
Member

@romanblanco In #3634, @abonas says that the Optional label "should go next to the field, not under the field."

@abonas
Copy link
Member

abonas commented Aug 10, 2015

@romanblanco @chessbyte I looked at how it looks and behaves in other providers, and the ones I saw (rhev) have some sort of statement under credentials/authentication part (required/optional) and I think the following should apply:
Required fields (user/password in infra provider) should be marked as required in a standard way - with a star next to them, in red, etc. Even saying "required", but next to them, not under.
Optional fields should not be marked at all - otherwise any optional field should say "optional" next to it and it's redundant.
cc @simon3z @oschreib @dclarizio

@romanblanco
Copy link
Member Author

@chessbyte I see. I was following BZ description, which says:
Expected results:
Should not show the "Optional." label

It also looks better to me without label. I would avoid using textual labels of which fields are required and which are optional, since user is notified about required fields through flash messages:

screenshot-localhost 3000 2015-08-11 00-25-25

@abonas @dclarizio

@dclarizio
Copy link

@abonas I was under the impression from the way the bug was written that we didn't want the text "Optional" shown on the screen. If this is for the Credentials, then perhaps it should say "Credentials (optional)" in the section title. Just let us know what we want to do here. Thx, Dan

@dclarizio
Copy link

@abonas Sorry, my screen hadn't been updated with your reply. For this screen, since it is not yet updated to use angular, I think something like I suggested is fine for now. Once we convert it, anything required that is not filled in will be in red with a small message below (yes, patternfly shows them below) it saying "Required" (see the schedule editor, once we fix it, of course, lol). Optional fields will not be highlighted.

@abonas
Copy link
Member

abonas commented Aug 10, 2015

@abonas I was under the impression from the way the bug was written that we didn't want the text "Optional" shown on the screen. If this is for the Credentials, then perhaps it should say "Credentials (optional)" in the section title. Just let us know what we want to do here. Thx, Dan

Indeed, generally speaking, I don't see the need in saying "optional".
My point was, that originally this bug was introduced afaik (cc @oschreib ) because there's a very similar label in infra providers, in the same location under the credentials dialog, which says "required section...". So I'm OK with removing the "optional" label here, or at least moving it to a bit more standard location not under the fields, but I also recommend to handle the required sections in a more standard way as I outlined above.

dclarizio pushed a commit that referenced this pull request Aug 12, 2015
Removed Optional. label from 'Add New Container Provider' screen
@dclarizio dclarizio merged commit 7bd18a8 into ManageIQ:master Aug 12, 2015
@dclarizio dclarizio added this to the Sprint 28 Ending August 24, 2015 milestone Aug 12, 2015
@romanblanco romanblanco deleted the BZ1248547 branch August 13, 2015 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants