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

Replace angular-based fonticon picker with a react component #6985

Merged
merged 2 commits into from
Apr 27, 2020

Conversation

skateman
Copy link
Member

@skateman skateman commented Apr 20, 2020

My original plan was to just copy-paste the component from react-ui-components to be compliant with #6716, but there were some missing and broken parts:

  • no way to preselect an icon
  • icons rendered as component -> ff doesn't support that
  • icons organized in a table 😢 🙈 yes you heard that right
  • icon fetch from CSS was building a table
  • no scrollbar when too many icons, it makes the whole screen scrolling
  • modal was too narrow
  • kinda complicated iconTypes specification schema

So I addressed these problems, mostly by just moving some stuff from my original angular component.

The custom buttons that require this component are being used in Automate Customization and also in Generic Objects/Catalogs. The solution for the first one was trivial, but the latter is fully implemented in angular 🦛 and it either has to be converted to react or some kind of hack is necessary to get rid of the angular-based fonticon picker. Anyway, the first part is totally ready for review 😉

Resolves #5758

@miq-bot add_reviewer @ZitaNemeckova
@miq-bot assign @himdel

@skateman
Copy link
Member Author

As #6910 has too many unresolved dependencies, I'm marking this as ready, so we can at least get rid of one dependency from react-ui-components.

@skateman skateman changed the title [WIP] Replace angular-based fonticon picker with a react component Replace angular-based fonticon picker with a react component Apr 21, 2020
@miq-bot miq-bot removed the wip label Apr 21, 2020
onClick={onModalApply}
disabled={selectedIcon === activeIcon || activeIcon === undefined}
>
Apply
Copy link
Contributor

Choose a reason for hiding this comment

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

__('Apply')

Apply
</Button>
<Button id="cancel-icon-picker-modal" bsStyle="default" className="btn-cancel" onClick={hideModal}>
Cancel
Copy link
Contributor

Choose a reason for hiding this comment

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

__('Cancel')

@skateman skateman force-pushed the icon-picker-react branch from 599807f to fea914e Compare April 22, 2020 10:12
@skateman skateman force-pushed the icon-picker-react branch 2 times, most recently from 89a5ac1 to 98b3b38 Compare April 22, 2020 12:57
@himdel
Copy link
Contributor

himdel commented Apr 22, 2020

Ah, one more thing..

app/views/shared/buttons/_ab_options_form.html.haml:102:   miq_bootstrap('#button-icon-picker');
app/views/shared/buttons/_group_form.html.haml:53:  miq_bootstrap('#button-icon-picker');

those are no longer needed.

EDIT: Oh, and both also have an onchange handler on the same element.. quite obsolete too .. except it suggest we may want to use miqObserveRequest instead of miqJqueryRequest

@skateman
Copy link
Member Author

skateman commented Apr 22, 2020

@himdel I'm worried that we don't use this miqObserveRequest anywhere in react, at least I haven't found any examples. So I guess we should go through all the stuff ...

EDIT: there's nothing relevant, at least in JSX files
EDIT2: and nowhere else under app/javascript

@skateman skateman force-pushed the icon-picker-react branch from 98b3b38 to b43c312 Compare April 22, 2020 13:15
@himdel
Copy link
Contributor

himdel commented Apr 22, 2020

I'm fine with either, this can only really affect automated testing or poor network condition (submit may not wait for the onchange event, to be processed).
But they also have the same api :).

@miq-bot
Copy link
Member

miq-bot commented Apr 22, 2020

Checked commits skateman/manageiq-ui-classic@9b329b3~...b43c312 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 4 offenses detected

app/views/shared/buttons/_ab_options_form.html.haml

  • ⚠️ - Line 49 - Avoid using instance variables in partials views
  • ⚠️ - Line 49 - Style/MethodCallWithArgsParentheses: Use parentheses for method calls with arguments.

app/views/shared/buttons/_group_form.html.haml

  • ⚠️ - Line 35 - Avoid using instance variables in partials views
  • ⚠️ - Line 35 - Style/MethodCallWithArgsParentheses: Use parentheses for method calls with arguments.

@himdel
Copy link
Contributor

himdel commented Apr 24, 2020

2 bugs:

Clicking the select doesn't do anything, only the chevron expands the dialog now.

The first 4 Font Awesome icons are:

fa .ff.ff-network-interface::before
fa .glyphicon-facetime-video::before
fa .glyphicon-fast-backward::before
fa .glyphicon-fast-forward::before

I'd say a parser bug, but glyphicon amongst font awesome icons sounds like more than just a parser bug..

@skateman
Copy link
Member Author

Clicking the select doesn't do anything, only the chevron expands the dialog now.

I probably don't understand what you are describing here, because for me everything works.

I'd say a parser bug

It was a parser bug, fixing it.

There were some wrongly implemented stuff in the original component:
* no way to preselect an icon
* icons rendered as component -> ff doesn't support that
* icons organized in a table
* no scrollbar when too many icons
* modal was too narrow
* icon fetch from CSS was building a table
* kinda complicated iconTypes specification schema

All these have been fixed in this single commit, most of the changes
have been inspired by the original angular component.
@skateman skateman force-pushed the icon-picker-react branch from b43c312 to e37e69c Compare April 27, 2020 12:34
@himdel
Copy link
Contributor

himdel commented Apr 27, 2020

Clicking the select doesn't do anything, only the chevron expands the dialog now.

I probably don't understand what you are describing here, because for me everything works.

20200427123900 mp4

EDIT: fixing in #7001

@skateman
Copy link
Member Author

Oh, this is standard behavior, same happens on upstream.

@himdel
Copy link
Contributor

himdel commented Apr 27, 2020

OK, sounds like this works :).

It does not replace the angular icon picker yet (%miq-fonticon-picker), that will be a separate PR, but it replaces all its uses in non-angular forms.

@himdel himdel merged commit c97f045 into ManageIQ:master Apr 27, 2020
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.

Replace Icon picker with a React one
5 participants