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

Use the carbon component mapper for our DDF forms #7464

Merged
merged 11 commits into from
Feb 15, 2021

Conversation

skateman
Copy link
Member

@skateman skateman commented Oct 29, 2020

This replaces the old PF3 component mapper with the Carbon one, basically making all our DDF-based forms appear as Carbon forms. We have custom components that are appended to this mapper which should be addressed one by one:

  • code-editor (reimplemented)
  • field-array (provided by the mapper)
  • dual-group (removed here)
  • dual-list-select (provided by the mapper)
  • password-field (UX?)
  • edit-password-field (UX?)
  • tree-view
  • tree-selector (UX?)
  • select
  • protocol-selector (inherited from select)
  • validate-credentials
  • provider-credentials (no PF deps)
  • validate-provider-credentials (inherited from validate-credentials)
  • detect-button (UX?)

There's also a styling issue which is probably a collision with our stylesheets pulled in from Carbon. The demo provided by DDF uses a CDN link for the stylesheets so we might want to investigate the differences.

Resolves #7278

@skateman
Copy link
Member Author

Right now the form looks like this:
Screenshot from 2020-10-29 13-54-58

If I switch the theme to g10 in the app/stylesheet/carbon.scss, it looks a little bit better, but it alters the menu to white:
Screenshot from 2020-10-29 13-50-34

I guess we have dark mode enabled?! @kavyanekkalapu @himdel

@kavyanekkalapu
Copy link
Member

Yes we should use g10 light mode and we can fix leftnav colors manually.

@skateman
Copy link
Member Author

@kavyanekkalapu I started playing with the menu and its styling and I think we might be using the wrong one. There's a bx--navigation which is always dark independently from the theme and a bx--side-nav which follows the color scheme from the theme. Right now we're using the bx--side-nav, but is it possible that it's the wrong one?

@skateman skateman force-pushed the carbon-component-mapper branch from e7de032 to 2dc5a1d Compare October 29, 2020 15:25
@himdel
Copy link
Contributor

himdel commented Nov 3, 2020

Ah, a different theme can be enabled for different parts of the app, by only @importing the theme under a selector, instead of globally.

Failing that, we can trivially switch to a white theme, the menu can be de-themified by running perl -i -npe 's/: \$.*\/\/ /: \$/ && s/$/;/' app/stylesheet/menu-colors.scss.
(The original idea was that productization could just switch the theme, but since we're not using a different theme there, we don't need to use a theme for the menu.)

@skateman skateman force-pushed the carbon-component-mapper branch from 2dc5a1d to 29dc29f Compare November 6, 2020 12:07
@skateman
Copy link
Member Author

skateman commented Nov 6, 2020

Since #7479 is in, we are now with the correct theme:
Screenshot from 2020-11-06 13-06-41

Isn't it beautiful? 😻

@skateman
Copy link
Member Author

skateman commented Nov 6, 2020

Blocked by data-driven-forms/react-forms#884

@skateman skateman force-pushed the carbon-component-mapper branch 2 times, most recently from 3d5ec62 to 98d3989 Compare November 12, 2020 19:08
@skateman skateman force-pushed the carbon-component-mapper branch 3 times, most recently from 6be2af6 to a107273 Compare November 13, 2020 13:16
@skateman
Copy link
Member Author

CodeEditor

Just changed the radio buttons and the group around them
Screenshot from 2020-11-13 13-44-25

DualGroup

Switched to the implementation provided by the component mapper, that has a few caveats. The buttons don't have a tooltip setting, so I had to put text on them to be a11y compliant. There's a filter for both columns, which I find a little bit ugly and I might create an upstream PR that allows us to hide it. Also there's no way to hide some of the buttons as it was possible in the original implementation (which is IMO fine).
Screenshot from 2020-11-13 13-48-30

(Edit)PasswordField

There's no input group (input addon) functionality in Carbon, so I don't know where to put the buttons for now. Asked UX to make my naive implementation a little bit better, waiting for their response.
Screenshot from 2020-11-13 13-59-07
Screenshot from 2020-11-13 13-59-16

AsyncCredentials

Not sure about the button style, need to talk with UX.
Screenshot from 2020-11-13 14-17-35
Screenshot from 2020-11-13 14-00-08
Screenshot from 2020-11-13 14-16-55

@skateman
Copy link
Member Author

Hangs on data-driven-forms/react-forms#896

@skateman skateman force-pushed the carbon-component-mapper branch 2 times, most recently from 75e2dbc to 9f082f9 Compare November 25, 2020 08:03
@skateman skateman force-pushed the carbon-component-mapper branch from 9f082f9 to 543a3bf Compare December 10, 2020 12:24
@skateman
Copy link
Member Author

UX got back to me with the input addons:
image

Unfortunately these aren't implemented as a react component, so we have to do it on our own in the DDF mapper.

data-driven-forms/react-forms#912

@skateman skateman force-pushed the carbon-component-mapper branch from 543a3bf to 8454084 Compare January 18, 2021 12:55
@kavyanekkalapu
Copy link
Member

kavyanekkalapu commented Jan 21, 2021

Aahh, I think I found it. The theme is loading properly and the colors are entirely consistent with Gray 10.

As per https://www.carbondesignsystem.com/components/form/style#color , the default input background is $field-01.
As per https://www.carbondesignsystem.com/guidelines/color/usage/, $field-01 is only gray in the White theme, it's white in the Gray 10 theme.

Carbon documentation uses the white theme, so any screenshots from carbon docs don't apply, we're using a different theme.

@himdel in that case carbon g10 form should look like this, we need some differentiation from BG to input fields or we can change theme to white to match with other non carbon pages in MIQ.

Screen Shot 2021-01-21 at 11 28 49 AM

@himdel
Copy link
Contributor

himdel commented Jan 21, 2021

Ah, then I suppose the question is, should we go with the white theme instead, or change the background color.

Presumably there's a form class that does that though? If not, looks like the background color to use is $ui-background

@himdel
Copy link
Contributor

himdel commented Jan 22, 2021

diff --git a/app/stylesheet/carbon.scss b/app/stylesheet/carbon.scss
index 02b908287c..544085d254 100644
--- a/app/stylesheet/carbon.scss
+++ b/app/stylesheet/carbon.scss
@@ -14,7 +14,7 @@ $css--default-type: false;
 
-// Use the gray 90 theme
+// Use the white theme
 @import '~@carbon/themes/scss/themes';
-$carbon--theme: $carbon--theme--g10;
+$carbon--theme: $carbon--theme--white;
 @include carbon--theme();
 
 @import '~carbon-components/scss/globals/scss/styles.scss';

@skateman skateman force-pushed the carbon-component-mapper branch 2 times, most recently from 48ae8f2 to df1e19c Compare January 26, 2021 13:54
@skateman
Copy link
Member Author

Okay, I (partially) addressed the issues, the dual-list should be fixed in the upstream package independently from this PR. I am looking for functional errors, like something not working that worked before so please do a sweep of the forms...

@kavyanekkalapu
Copy link
Member

@skateman i verified the branch,

  • When i enter something in code editor, i see this message, but my json has many syntax issues, it did not highlight them.

Screen Shot 2021-02-01 at 9 03 14 AM

  • I think, we need some spacing between options and editor

Screen Shot 2021-02-01 at 8 58 45 AM

  • Editor should be in other BG color may be same like input text fields.

@skateman
Copy link
Member Author

skateman commented Feb 1, 2021

@skateman i verified the branch,

  • When i enter something in code editor, i see this message, but my json has many syntax issues, it did not highlight them.
Screen Shot 2021-02-01 at 9 03 14 AM

Because the code editor doesn't do error highlighting, only syntax coloring, you can verify that in the patternfly version on master.

  • I think, we need some spacing between options and editor
Screen Shot 2021-02-01 at 8 58 45 AM
  • Editor should be in other BG color may be same like input text fields.

This is a styling issue

@skateman skateman force-pushed the carbon-component-mapper branch from df1e19c to 2e9bda6 Compare February 8, 2021 09:56
@miq-bot
Copy link
Member

miq-bot commented Feb 8, 2021

Checked commits skateman/manageiq-ui-classic@60c2a81~...2e9bda6 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@h-kataria
Copy link
Contributor

@skateman tested in UI, looking good to me other than styling issues. What is the next step?

@skateman
Copy link
Member Author

skateman commented Feb 9, 2021

@skateman tested in UI, looking good to me other than styling issues. What is the next step?

lol, I guess to merge? 🤣

@h-kataria
Copy link
Contributor

@skateman tested in UI, looking good to me other than styling issues. What is the next step?

lol, I guess to merge? 🤣

wasn't sure if @kavyanekkalapu is still testing or found any other issues. @kavyanekkalapu are you still testing this or are you good with merge?

@skateman
Copy link
Member Author

Extracted the styling issues into a separate issue so this can be safely merged.

@h-kataria h-kataria merged commit 994b17e into ManageIQ:master Feb 15, 2021
@skateman skateman deleted the carbon-component-mapper branch February 15, 2021 14:51
@chessbyte chessbyte added this to the Lasker milestone Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DDF componentMapper for Carbon
6 participants