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 react warning on visiting add page and align radio button sections in git import and deploy image forms with current UXD #3372

Merged
merged 4 commits into from
Nov 16, 2019

Conversation

divyanshiGupta
Copy link
Contributor

@divyanshiGupta divyanshiGupta commented Nov 13, 2019

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. component/dev-console Related to dev-console labels Nov 13, 2019
@gijohn
Copy link
Contributor

gijohn commented Nov 13, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2019
@debsmita1
Copy link
Contributor

@divyanshiGupta The radio button is being used in the Deploy Image form as well. Don't you think the position of the input field looks a bit off?

deploy image

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2019
@divyanshiGupta
Copy link
Contributor Author

@divyanshiGupta The radio button is being used in the Deploy Image form as well. Don't you think the position of the input field looks a bit off?

deploy image

Yes, it was because br was removed after radio button that was creating extra whitespace in resource section. Fixed the issue by using a custom class instead.
image

@divyanshiGupta
Copy link
Contributor Author

divyanshiGupta commented Nov 13, 2019

/cc @openshift/team-devconsole-ux
/cc @serenamarie125

@openshift-ci-robot
Copy link
Contributor

@divyanshiGupta: GitHub didn't allow me to request PR reviews from the following users: /cc.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @openshift/team-devconsole-ux /cc @serenamarie125

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@andrewballantyne
Copy link
Contributor

/assign
/hold

I'm sorry to have to do this to you @divyanshiGupta but I think we need to make a few changes here to clean this up more. It'll expand out a bit from what you have done. I'll compile the list and comment back when I have it.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2019
@divyanshiGupta
Copy link
Contributor Author

@andrewballantyne sure, no problem. I have tagged UX for the same reason so that it can be confirmed what all we need to add/change.

@andrewballantyne
Copy link
Contributor

@divyanshiGupta

Okay, so there are 3 graphical issues and 1 code issue I can see.

The visual issues (@openshift/team-devconsole-ux)
image
image
image

The code issue

export interface RadioOption {
  value: string;
  label: React.ReactNode;
  helperText?: React.ReactNode;
  displayField?: React.ReactElement;
}

helperText and displayField are essentially giving the same content for use in a children-esk way. I think we need to consolidate some of this logic, but the way displayField is used, we'll need to handle that as well.

I'll follow up with some thoughts on how to address these issues, but for the most part - this is what I think needs to improve as part of this ticket so we can align better with UX and achieve a smoother use of the RadioButtonField component.

@andrewballantyne
Copy link
Contributor

Fixing the code issue is relatively simple... a rebranding of props and a more consolidated rendering will get us a cleaner implementation.

Suggest making use of first-class naming in React and addressing the content as children:

export interface RadioOption {
  value: string;
  label: React.ReactNode;
  children?: React.ReactNode;
  activeChildren?: React.ReactElement;
}

activeChildren will be necessary unfortunately to allow for content to appear and disappear as the radio buttons are selected. Whereas whenever you just want static children content, you can just use children. We'd have to support the possibility of both, but as long as the static children go first, I don't see any issues moving forward.

I think we can clean up things and just render all the children (as-needed) after the radio button, indenting the children content all at once to align it with the radio buttons:

      {options.map((option) => {
        const activeChild = field.value === option.value && option.activeChildren;
        const staticChild = option.children;

        return (
          <React.Fragment key={option.value}>
            <Radio
              {...field}
              {...props}
              id={getFieldId(option.value, 'radiobutton')}
              value={option.value}
              label={option.label}
              isChecked={field.value === option.value}
              isValid={isValid}
              aria-describedby={`${fieldId}-helper`}
              onChange={(val, event) => {
                field.onChange(event);
              }}
            />
            {(activeChild || staticChild) && (
              <div className="odc-radio-button__children">
                {staticChild}
                {activeChild}
              </div>
            )}
          </React.Fragment>
        );
      })}

@andrewballantyne
Copy link
Contributor

The style issues for the radio buttons is a bit annoying from what I can see. The radio circle itself is not centred despite Patternfly's attempt at doing so. This appears to be an issue with bootstrap styles conflicting with Patternfly styles. There is the following style at play:

// public/node_modules/bootstrap-sass/assets/stylesheets/bootstrap/_forms.scss
input[type="radio"], input[type="checkbox"] {
    margin: 4px 0 0;
}

Which misaligns the circle down a few pixels making the centring logic of Patternfly not work out.

I recommend we just override it by adding a class to the RadioButtonField's FormGroup:

.odc-radio-button input[type=radio] {
    margin-top: 0;
}

Which will allow Patternfly to centre based on their flex align-items logic.

@andrewballantyne
Copy link
Contributor

The badge is a problem on its own... it takes up more space than the label does. The easiest way I can think of solving this would be for us to just make it take up no space and "be centred".

Now this can be done by the use of the css position property. We can "anchor" (position: relative) our badge locally so that it's inline with the label, but tell it to position irrespective of its layout location (position: absolute). This gives us the opportunity to position it relative to the anchor without it informing the anchor of its size (essentially, taking up zero space).

CSS would be something like:

.odc-resource-section {
  &__badge-wrapper {
    // we could use a span for this as well, this just says "I'll be inline with the text"
    display: inline;
    margin-left: var(--pf-global--spacer--md);
    position: relative; // our "anchor"
  }

  &__inline-badge {
    bottom: -25%; // align it centred
    left: 0; // keep it aligned relative to our "anchor"
    position: absolute; // position outside of the layout, aka be of no-size
  }
}

Which would yield something like:
image

@andrewballantyne
Copy link
Contributor

/hold cancel
/retitle [WIP] Fix react warning on visiting add page and remove unnecessary white space in resource section

If you have any questions, please let me know @divyanshiGupta - I've tossed a WIP onto your PR while you work out these changes.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2019
@openshift-ci-robot openshift-ci-robot changed the title Fix react warning on visiting add page and remove unnecessary white space in resource section [WIP] Fix react warning on visiting add page and remove unnecessary white space in resource section Nov 13, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2019
@rohitkrai03
Copy link
Contributor

rohitkrai03 commented Nov 13, 2019

@andrewballantyne I added this comment to Karthik's PR some time back -

This component has become too imperative for my liking. I would prefer a decalartive API like -

<RadioButtonField name="registry" label="Image Section">
  <RadioButtonOption
    label={imageRegistryType.External.label}
    value={imageRegistryType.External.value}
    helpertext="Some help text"
  >
    <ImageSearch />
  </RadioButtonOption>
  <RadioButtonOption
    label={imageRegistryType.Internal.label}
    value={imageRegistryType.Internal.value}
    helpertext="Some help text"
  >
    <ImageStream projects={projects} imageStreams={imageStreams} />
  </RadioButtonOption>
</RadioButtonField>

I think we should definitely look at improving this API in future.

We had a bug for addressing some of the issues but later we decided we'll handle it as a tech debt. I see that we are already looking for some refactoring here so thought why not toss this idea in. I think if we simplify the APIs of RadioButtonField and make it more declarative it would solve both of the issues you mentioned above. This way we can have a helperText as required and only render children when the radio option is active. What do you think about this approach?

@andrewballantyne
Copy link
Contributor

We had a bug for addressing some of the issues but later we decided we'll handle it as a tech debt. I see that we are already looking for some refactoring here so thought why not toss this idea in. I think if we simplify the APIs of RadioButtonField and make it more declarative it would solve both of the issues you mentioned above. This way we can have a helperText as required and only render children when the radio option is active. What do you think about this approach?

Definitely an interesting prospect. We could go down this path. If we go more declarative, I'd prefer if we expanded the rendering to allow for a React Node or a function. That way the function can echo back if it's selected, and you can decide if you want to render it.

Given your example, it could be something like this:

<RadioButtonField name="registry" label="Image Section">
  <RadioButtonOption
    label={imageRegistryType.External.label}
    value={imageRegistryType.External.value}
    helpertext="Some help text"
  >
    {(isChecked) => isChecked && <ImageSearch />}
  </RadioButtonOption>
  <RadioButtonOption
    label={imageRegistryType.Internal.label}
    value={imageRegistryType.Internal.value}
    helpertext="Some help text"
  >
    {(isChecked) => isChecked && (
      <ImageStream projects={projects} imageStreams={imageStreams} />
    )}
  </RadioButtonOption>
</RadioButtonField>

Would definitely be an interesting opportunity to address this. It would require a bigger interface modification than I had hoped for. Perhaps we push this to tech debt for the time being.

@divyanshiGupta
Copy link
Contributor Author

@andrewballantyne @rohitkrai03 thanks for the suggestions. I was also thinking of refactoring this code but it seemed like a big refactor that is why I avoided doing it in this PR and wanted to keep this PR for only cosmetic changes. I can create a ticket for the refactoring work in Jira and raise a separate PR.

@sunilmalagi
Copy link

sunilmalagi commented Nov 14, 2019

@divyanshiGupta

Okay, so there are 3 graphical issues and 1 code issue I can see.

The visual issues (@openshift/team-devconsole-ux)
image
image
image

The code issue

export interface RadioOption {
  value: string;
  label: React.ReactNode;
  helperText?: React.ReactNode;
  displayField?: React.ReactElement;
}

helperText and displayField are essentially giving the same content for use in a children-esk way. I think we need to consolidate some of this logic, but the way displayField is used, we'll need to handle that as well.

I'll follow up with some thoughts on how to address these issues, but for the most part - this is what I think needs to improve as part of this ticket so we can align better with UX and achieve a smoother use of the RadioButtonField component.

@divyanshiGupta @andrewballantyne @serenamarie125
Divyanshi, you are right there should be indentation, here is the reference for the same, (visuals created for 'Deploy Image'): https://marvelapp.com/7i6ghc5 and I would advice you to follow the same.
For the radio button, I too see inconsistency in alignment, there is a component in Pattern-fly for radio button, see if it helps you https://www.patternfly.org/v4/documentation/core/components/radio

@divyanshiGupta
Copy link
Contributor Author

For the radio button, I too see inconsistency in alignment, there is a component in Pattern-fly for radio button, see if it helps you https://www.patternfly.org/v4/documentation/core/components/radio

@sunilmalagi We are using pf radio button already. Will fix the issue by using a custom class to override the button margin.

@divyanshiGupta
Copy link
Contributor Author

@andrewballantyne @sunilmalagi I have addressed the requested changes. PTAL.
Deploy Image
image
image
Also removed image section label since it is not shown in the UX doc https://docs.google.com/document/d/1OxnQE-49dkww_6x_0u1bMKi5j1VGggka7zL78hMrxgM/edit?ts=5d920365#heading=h.jykc4dkvr8hr
Git import
image

@divyanshiGupta Thanks for updating Looks good to me, small tweak is still needed for 'Resources' section.. may be text should be moved towards up couple off pixels..

@sunilmalagi can you please specify which text?

@sunilmalagi
Copy link

@andrewballantyne @sunilmalagi I have addressed the requested changes. PTAL.
Deploy Image
image
image
Also removed image section label since it is not shown in the UX doc https://docs.google.com/document/d/1OxnQE-49dkww_6x_0u1bMKi5j1VGggka7zL78hMrxgM/edit?ts=5d920365#heading=h.jykc4dkvr8hr
Git import
image

@divyanshiGupta Thanks for updating Looks good to me, small tweak is still needed for 'Resources' section.. may be text should be moved towards up couple off pixels..

@sunilmalagi can you please specify which text?

@divyanshiGupta
alignment with radio buttons and 'Deployment' 'Deployment Config' & 'Knative Service' text needs to adjusted

@andrewballantyne
Copy link
Contributor

andrewballantyne commented Nov 14, 2019

@andrewballantyne @rohitkrai03 thanks for the suggestions. I was also thinking of refactoring this code but it seemed like a big refactor that is why I avoided doing it in this PR and wanted to keep this PR for only cosmetic changes. I can create a ticket for the refactoring work in Jira and raise a separate PR.

Unfortunately, you do need to do this change: #3372 (comment)
(or something similar)

The radio buttons need to align themselves with the first line of text:
image

This is happening because to solve the issue of alignment in the previous design, Giftson top-aligned the radio button... and made use of the bootstrap styles to push the radio button down - but this doesn't align with the other use of radio buttons.

This isn't a standard way of us addressing radio buttons. Patternfly wants to vertically align it in the label, and our label right now can be a multi-line label:

label={
  option.helperText ? (
    <>
      {option.label}
      <div className="odc-radio-button__helper-text">{option.helperText}</div>
    </>
  ) : (
    option.label
  )
}

This field cannot be used with the current styles. It'll never align. See the styles for the resources section for our override that will need to be removed.

Also, when completing redesigns of UI work, could you post an updated screenshot - it'll be easier to give feedback and it allows the author of the PR to get one last look at their work before they ask others to verify it. And you did... not sure how I didn't see that when I came back to do my review. Apologizes.

Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Here is my review on the code.

Please let me know if you have any questions or concerns.

Comment on lines 67 to 68
<div className="odc-resource-section__badge-wrapper">
<span className="odc-resource-section__inline-badge">
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps make both of these spans, since the styles:

  &__badge-wrapper {
    display: inline;

Are just getting rid of the divs block nature anyways. And divs for the most part are nothing but block containers.

}
.odc-resource-section {
&__badge-wrapper {
display: inline;
Copy link
Contributor

Choose a reason for hiding this comment

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

Per my other comment, perhaps remove this line.

Comment on lines 5 to 11
.odc-radio-button input[type='radio'] {
margin-top: 0;
}

.odc-radio-button__helper-text {
padding: var(--pf-global--spacer--md) 0;
line-height: 1em;
}

.odc-radio-button__displayed-field {
padding: var(--pf-global--spacer--md) 0 var(--pf-global--spacer--md) 1.8rem;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update this, making use of our SCSS style. See frontend/packages/dev-console/src/components/import/section/ResourceSection.scss for an example of how to nest items.

If you're unfamiliar with the inner workings of SCSS and the parent selector, please see https://sass-lang.com/documentation/style-rules/parent-selector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reference. Was not sure if we can nest selectors as well.

Copy link
Contributor

@andrewballantyne andrewballantyne Nov 14, 2019

Choose a reason for hiding this comment

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

Ah yeah, I guess documentation only goes so far.

So the way SCSS nesting works is:

.my-class {
  .my-inner-class { }

  &.my-modifier { }

  input[type=radio] { }
}

Gives us this CSS in the end (if there were properties inside them):

// top-level properties for the first dom element
.my-class {}

 // two dom elements, one nested inside the other
.my-class .my-inner-class {}

 // one dom element with a modifier class
.my-class.my-modifier {}

 // two dom elements, inner one being an radio input
.my-class input[type=radio] {}

& is merely append to the selector (aka don't use a space)

@divyanshiGupta
Copy link
Contributor Author

@andrewballantyne @rohitkrai03 thanks for the suggestions. I was also thinking of refactoring this code but it seemed like a big refactor that is why I avoided doing it in this PR and wanted to keep this PR for only cosmetic changes. I can create a ticket for the refactoring work in Jira and raise a separate PR.

Unfortunately, you do need to do this change: #3372 (comment)
(or something similar)

The radio buttons need to align themselves with the first line of text:
image

This is happening because to solve the issue of alignment in the previous design, Giftson top-aligned the radio button... and made use of the bootstrap styles to push the radio button down - but this doesn't align with the other use of radio buttons.

This isn't a standard way of us addressing radio buttons. Patternfly wants to vertically align it in the label, and our label right now can be a multi-line label:

label={
  option.helperText ? (
    <>
      {option.label}
      <div className="odc-radio-button__helper-text">{option.helperText}</div>
    </>
  ) : (
    option.label
  )
}

This field cannot be used with the current styles. It'll never align. See the styles for the resources section for our override that will need to be removed.

Also, when completing redesigns of UI work, could you post an updated screenshot - it'll be easier to give feedback and it allows the author of the PR to get one last look at their work before they ask others to verify it.

@sunilmalagi ^^

@divyanshiGupta
Copy link
Contributor Author

@andrewballantyne @rohitkrai03 thanks for the suggestions. I was also thinking of refactoring this code but it seemed like a big refactor that is why I avoided doing it in this PR and wanted to keep this PR for only cosmetic changes. I can create a ticket for the refactoring work in Jira and raise a separate PR.

Unfortunately, you do need to do this change: #3372 (comment)
(or something similar)

@andrewballantyne Are you suggesting to do the refactor work in this PR?

@andrewballantyne
Copy link
Contributor

andrewballantyne commented Nov 14, 2019

@andrewballantyne @rohitkrai03 thanks for the suggestions. I was also thinking of refactoring this code but it seemed like a big refactor that is why I avoided doing it in this PR and wanted to keep this PR for only cosmetic changes. I can create a ticket for the refactoring work in Jira and raise a separate PR.

Unfortunately, you do need to do this change: #3372 (comment)
(or something similar)

@andrewballantyne Are you suggesting to do the refactor work in this PR?

I recommend doing my refactor or something similar... you need to get the label to be a single line item for Patternfly to do it's own thing.

EDIT: @divyanshiGupta if you can fix this issue cleverly without breaking the other use-case of radio, that'll also be acceptable.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 15, 2019
@divyanshiGupta
Copy link
Contributor Author

@andrewballantyne I have addressed all the requested changes. Refactored the code and did a little bit of css tweaks to resolve the alignment issues. PTAL.
image

@sunilmalagi ^^

@sunilmalagi
Copy link

@andrewballantyne I have addressed all the requested changes. Refactored the code and did a little bit of css tweaks to resolve the alignment issues. PTAL.
image

@sunilmalagi ^^

@divyanshiGupta Looks Good.. thank you for doing it..

@@ -22,12 +22,12 @@ type ResourceSectionProps = {

const createHelpText = (k8sModel: K8sKind, helpText: string) => {
return (
<>
<div style={{ lineHeight: '1em' }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

The easiest way to know when to use lineHeight is (almost) never :)

What you have done here is take 2 paragraphs and try to condense them down to 1 font-size in line height. The browser is stopping you from doing this to the full extent :)

What was the goal here? Just to make them more condensed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because in UXD it seems the spacing between them is less than what is shown here by default. So thought of reducing line height. You are right though, its more of a hack :P

Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend going with a more css approach, turning both <p> into <div>s and using a Patternfly spacer var(--pf-global--spacer--sm) on the top one. Instead of a line-height :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

@@ -23,10 +23,10 @@ type ResourceSectionProps = {
const createHelpText = (k8sModel: K8sKind, helpText: string) => {
return (
<>
<p>
<div style={{ paddingBottom: 'var(--pf-global--spacer--xs)' }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

lol - use your SCSS file. Inline should be avoided in all cases possible.

Copy link
Contributor Author

@divyanshiGupta divyanshiGupta Nov 15, 2019

Choose a reason for hiding this comment

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

Yes I can do that but here it doesn't seem necessary because it is only a single style, also this is only targeted at this and is not used anywhere else. Inline styles are being used in a lot of other places in console.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add inline css when no more than one style is required and it is not going to be used anywhere else. In this case having a inline css or a class seemed to have equal weight. Updated anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't agree; inline styles are a poor way to write maintainable code. If UX came back and asked us to add more styles (colour the resource name, for instance), you don't have a nice spot to put them, you have to refactor your styles into a css file and then add the new styles (or just clutter up the React code some more). Static styles (imo) have no purpose being written inline.

It's convenient to use the style attribute, I understand that, but we are not writing code that we're going to toss out. We're writing code we'll have to maintain; best write it in such a fashion that we don't have to rewrite for small additions.

Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2019
@divyanshiGupta
Copy link
Contributor Author

/assign @christianvogt

@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, divyanshiGupta, gijohn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2019
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@divyanshiGupta
Copy link
Contributor Author

/test e2e-gcp-console

@openshift-merge-robot openshift-merge-robot merged commit ef0548e into openshift:master Nov 16, 2019
@spadgett spadgett added this to the v4.3 milestone Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.