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

Fix create namespace modal #1219

Merged
merged 11 commits into from
Nov 25, 2021

Conversation

MilanPospisil
Copy link
Contributor

@MilanPospisil MilanPospisil commented Nov 9, 2021

From Issue AAH-824:

  • Resize modal to standard (shorter width)

  • Remove "please, provide a namespace" text under "name" field

  • Move popover question mark (input group) to form (next to field label)

  • Reword "name" field alert message from "Name can only contain [A-Za-z0-9_]" to "Name can only contain letters and numbers"

  • Remove popover extra right padding

  • Change "namespace owners" select prompt from "find a group" to "select a group", let selected groups appear in field as chips

  • Add "Select namespace owner permissions" title above permissions section

Completed: 1, 2, 4, 6 (partialy), 7

I was not sure what 3, 5 and part of 6 ment to be, maybe it is already corrected.

@himdel
Copy link
Collaborator

himdel commented Nov 9, 2021

the test failures look relevant, probably want to update the tests :)

@himdel
Copy link
Collaborator

himdel commented Nov 16, 2021

  • Resize modal to standard (shorter width)
before after
20211116043958 20211116043847
  • Remove "please, provide a namespace" text under "name" field

  • Move popover question mark (input group) to form (next to field label)

  • Reword "name" field alert message from "Name can only contain [A-Za-z0-9_]" to "Name can only contain letters and numbers"

  • Remove popover extra right padding

    • This refers to the right padding next to the text in the popover... 20211116045520
    • We're using our HelperText component which ends up using the patternfly Popover component.
    • If you look at the docs, visible more in the HTML tab, but also present in the React tab, there seem to be 2 variants, one with a header, where the padding is only on the header, not the body, and the one we use. So .. I think we can just switch our HelperText to use the other variant, somehow ;)
  • Change "namespace owners" select prompt from "find a group" to "select a group", let selected groups appear in field as chips

    • Change "namespace owners" select prompt from "find a group" to "select a group"
    • let selected groups appear in field as chips
      • so, what this means is exactly what you see with permissions .. but it doesn't work at all with reducing the width of the modal, I'd consider this obsolete, we can ask Susan
      • => obsolete, never mind
  • Add "Select namespace owner permissions" title above permissions section

    • this doesn't seem resolved, you've changed the placeholder instead of adding a title... I guess this is a Susan question as well, not sure how such a title should look or where it should go (see below)
    • EDIT: no need to add a field label

@himdel
Copy link
Collaborator

himdel commented Nov 16, 2021

Unless @ZitaNemeckova disagress, I'd just remove the selectPermissionCaption changes, as those don't seem to fit the description; unremove that one "Please, provider the namespace name" (or replace it with "Name required" or something); and we can merge the rest, and address the unchecked point at the next UX meeting. (Up to you if the Popover fix goes in a separate PR or here, but it will probably take longer than the rest.)

@himdel
Copy link
Collaborator

himdel commented Nov 16, 2021

So, @MilanPospisil I talked with @sbuenafe-rh today, and we have couple of changes....

  • the modal width should go to medium, not small (mostly because the permission select needs the space)
  • "let selected groups appear in field as chips" is obsolete (from before you could set permissions for groups)
  • "title above permissions section" - Susan will get back to us on this one, but the idea was ta add a form label before the groups ;; EDIT: no need to add a title

@MilanPospisil
Copy link
Contributor Author

Remove padding in helper text now looks like this. But this change is for all helpers, is it that you wanted or should I go with some props options in helper component?
Screenshot_20211119_112655

@himdel
Copy link
Collaborator

himdel commented Nov 23, 2021

Remove padding in helper text now looks like this. But this change is for all helpers, is it that you wanted or should I go with some props options in helper component?

Yeah, for all helpers is good 👍

But it doesn't look that good yet:

20211123223559

The text is now too close to the close button vertically, I think we'd need to push the close button closer to the top right corner (and the same distance from each), and keep at least the same padding between the button and the text.


Possible options:

  • require title for each HelperText
 export class HelperText extends React.Component<IProps, {}> {
   render() {
-    let div = <div style={{ height: '10px' }}></div>;
+    let div = "Namespace name";
     return (

20211123222809

  • reduce close button padding
 export class HelperText extends React.Component<IProps, {}> {
   render() {
-    let div = <div style={{ height: '10px' }}></div>;
+    let div = <div></div>;
     return (

and adding something like this css on the close button...

top: -2px;
right: -10px;

20211123222914

  • add padding on the other sides of content

(same empty div header, different css:)

.pf-c-popover__content { padding: 2rem; }

20211123223457

(but this one still doesn't look quite right)

Cc @sbuenafe-rh WDYT? :)

@MilanPospisil
Copy link
Contributor Author

MilanPospisil commented Nov 24, 2021

So I removed the title and fixed the layout in helper. I hope it is all, Im starting to get lost in so much issues in one PR :-) Maybe I should divide this kind of problems in two PRs in future.
Edit: Nope, I forgot tests :-)

Copy link
Collaborator

@himdel himdel left a comment

Choose a reason for hiding this comment

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

LGTM, I agee all the points are fixed now 👍

And we can talk about the proper way to solve all the other helper text paddings on the next UX meeting, the header you added works correctly here :)

@MilanPospisil MilanPospisil merged commit 5ec9af8 into ansible:master Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants