-
Notifications
You must be signed in to change notification settings - Fork 990
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 #7462 - new UI for network interfaces #1888
Conversation
@@ -0,0 +1,7 @@ | |||
|
|||
<div style="display: none;" id="interfaceHidden<%= local_assigns[:id] %>" interface-id="<%= local_assigns[:id] %>"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: use class=hide
Could you please add some space between green button "Add Interface" and horizontal line below? |
I'm not sure whether this issue is introduced by this PR but when I try to add new interface to existing host and fill in only identifier, it allows me to save the host, I see error but the interface is not created. Same applies to new host. |
Updated:
Screenshots updated too. |
@ares the issue you came across with is an error somewhere in validations (it's in development branch too). NICs with blank mac addresses are just removed. |
@tstrachota definitely much nicer! Last thing I doubt is the warning icon in edit button (sorry for potential bad hint, I didn't image varying width of button), let's see what others think. Don't worry about the validation issue then, validations may change later and it's not part of this PR. |
@tstrachota you made rubocop angry http://ci.theforeman.org/job/test_develop_pr_rubocop/1054/checkstyleResult/source.0/#332 :-) |
@ares I'm not fond of the varying size too. We can remove the icon. No strong opinion from my side here. |
IMHO the background color on error is too much. Can we just mark the tr border red (similar to other errors in input fields)? I think the icon next to edit on error is not needed.maybe change the btn class instead of an icon for f its needed? |
yes, I wonder if we should have the warning (above the table) wrapped in a |
looks better!, does the buttons look better when you pull-right them? |
Buttons on right seem too far from the rest of the content to me. |
@@ -0,0 +1,86 @@ | |||
|
|||
<% @interfaces_invalid = @host.interfaces.any?{ |i| !i.valid? } %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: could you please use interfaces_invalid without @ so we don't risk any conflict in future?
@tstrachota I'm happy with the code, could you please squash before I'll reapply and test it? |
@ares done |
<tr class="<%= 'has-error' unless interface.valid? %>" id="interface<%= interface.object_id %>" data-interface-id="<%= interface.object_id %>"> | ||
<td class="status hidden-xs" align="center"><%= link_status(interface) %></td> | ||
<td class="identifier ellipsis"><%= interface.identifier %></td> | ||
<td class="type hidden-xs"><%= interface.class.humanized_name %></td> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing N_()
Merged as 3154d77, thanks @tstrachota! |
Editing network interfaces moved into a modal window. Some screenshots of the new UI are at:
https://tstrachota.fedorapeople.org/screenshots/nics_ui/
Bits of host form related to NICs are initially hidden and copied into a modal window on edit button click. This allows reusing validation error notices rendered by rails.