networkmanager: Use TrashIcon instead of MinusIcon for field groups, and text-based add buttons instead of PlusIcon

[skobyda]

Fixes #19211

[garrett]

Thanks. This is better.

One thing in particular here: The + buttons should be really be text-based. "Add something" instead of (a badly misaligned) + icon.

Probably the following:

Add address
Add DNS server
Add search domain
Add route

[martinpitt]

/packit retest-failed

[martinpitt]

@garrett Do you want to block this on fixing the plus buttons, or is that okay for a follow-up?

I'm a bit surprised that this didn't seem to cause any pixel test failures.

[martinpitt]

FTR, we can ignore the tox failure, that is fixed in main and unrelated.

[skobyda]

Thanks. This is better.

One thing in particular here: The + buttons should be really be text-based. "Add something" instead of (a badly misaligned) + icon.

Probably the following:

Add address Add DNS server Add search domain Add route

@garrett, fixed:

[skobyda]

I'm a bit surprised that this didn't seem to cause any pixel test failures.

It feels like it definitely should have a pixel test. So I added one

[martinpitt]

Needs at least one more push:

test/verify/check-networkmanager-settings:99:50: E261 at least two spaces before inline comment

[martinpitt]

new pixels look good, thank you! The + icon looked nicer IMHO, less space and less to translate, but I trust @garrett 's judgement here.

So aside from the code style issue this LGTM!

[martinpitt]

Breaks TestNetworkingSettings.testNoConnectionSettings everywhere 😢

[martinpitt]

This pixel test failure looks weird. The mode is "DHCP/automatic", and yet it has manual address entry fields. Is that a regression?

[skobyda]

This pixel test failure looks weird. The mode is "DHCP/automatic", and yet it has manual address entry fields. Is that a regression?

You can set automatic DHCP + additional address with tha tmanual address entry. So no regression

[martinpitt]

Ah, this is because the third commit explicitly clicks on "Add address" to make this appear, I see.

[martinpitt]

This looks okay to me now. I'd like to get Garrett's sign-off on this, and this introduces new strings, so I'd like to hold it off for a bit. Unfortunately, translating the most recent 299 release takes a lot longer than usual 😢

[garrett]

The + versus "Add something" is not super clear on PatternFly:

• Icon buttons are supported in PF in general, but they seem to be a fallback when there isn't space, not a default: https://www.patternfly.org/components/button/design-guidelines#icon-button

• Content considerations have several points (from https://www.patternfly.org/components/button/design-guidelines#content-considerations):

  • Make sure that button text is unique and easily understood on its own
  • Avoid punctuation on buttons
  • Add an icon to the button to call a user's attention to it or to clarify the button's action
  • Do not create icons on buttons using punctuation (for example, "+"). Instead, refer to PatternFly's icons page for any icons you place on buttons.

• Form controls use "Add" as an example:

  • 

If we are to keep the + icons, they would have to be icons that would have to be properly centered and have tooltip and proper ARIA text. Having "Add" and what you're adding is more obvious overall. However, they're probably obvious visually by context.

The text does add visual noise (looks like a "jagged" "rough edge") and it breaks the alignment of other elements (switches).

While the - to trash icons are a super obvious change, seeing the screenshot makes me wonder about the + instead.

Would you, instead, be able to fix the alignment of the + icons and ensure they have tooltips and a proper aria-label?

[skobyda]

@garrett something like this:

[martinpitt]

Code-wise this looks okay -- it needs to actually add the pixel ref of course. Leaving the UI review to @garrett. Thanks!

[garrett]

That's better, for sure. There are still 2 pixels more on the bottom than the top, oddly. If it was shifted down a pixel, it'd be correct. (Sides are correct at least.)

(To help visualize this: The orange box is the same height as the space above the + icon.)

Is this a PF bug? Possibly.

Additionally, the icon isn't hinted; it has some odd subpixel going on. It's probably designed for another size. But this is either a PF issue or the icons they use.

Anyway, it looks correct in diff; these have to be PF issues. And this PR improves everything, so I'm approving. 👍

[garrett]

I was playing around with it further and did discover when you select "Manual", it shows a trash icon that's in a disabled state:

Shouldn't this trash icon not be there, instead of being disabled? Could you add this to the PR or as a follow-up?

It seems you can add more addresses? Should this be a "remove, but there must be one" situation? Or should there only be one? Or is it really the first one cannot be changed and you can remove additional?

I guess the scope for this should be in a follow-up.

[martinpitt]

I mopped up the new pixel test, thanks!