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

Expose our DDF renderer and constants as an alias for other engines #7005

Merged
merged 1 commit into from
May 18, 2020

Conversation

skateman
Copy link
Member

@skateman skateman commented Apr 28, 2020

We have our own renderer for DDF and it would be nice to expose it for other rails engines. This would allow me to continue in getting rid of final-form components in some provider repos by replacing them with DDF.

The alias is set to @@ddf, so we could load a component like this:

import MiqFormRenderer, { componentTypes, validatorTypes } from "@@ddf";

@miq-bot add_label enhancement, react
@miq-bot assign @himdel

@skateman
Copy link
Member Author

This would still require the provider plugins to depend directly on DDF, which we might want to avoid if we'd like to avoid problems when upgrading DDF to newer versions. However, the only thing we need from DDF are constants, like componentTypes and validatorTypes so I could just export them from the app/javascript/forms/data-driven-form.

I am also thinking about renaming the data-driven-form to something cleaner and we might just want to expose that one single file. What do you think @himdel?

@skateman skateman changed the title Expose app/javascript as an alias in webpack for other engines [WIP] Expose app/javascript as an alias in webpack for other engines Apr 29, 2020
@himdel
Copy link
Contributor

himdel commented Apr 29, 2020

Oh, nice, I had started something similar in #4966, but never finished making all the code use it.

I'm not sure we should use this in a plugin though. Tying a plugin to the exact structure of files in manageiq-ui-classic sounds brittle, I think plugins should only use the component registry.

(I'd still go with something shorter, @@ManageIQ is perhaps too long, I was originally thinking something like ~ui but that's probably problematic for sass.)

@skateman
Copy link
Member Author

@@miq-ui?

@skateman skateman force-pushed the expose-app-javascript branch from 815b5db to b6822b1 Compare April 29, 2020 09:41
@skateman skateman changed the title [WIP] Expose app/javascript as an alias in webpack for other engines Expose our DDF renderer and constants as an alias for other engines Apr 29, 2020
@@ -136,6 +136,7 @@ module.exports = {
'bootstrap-select': '@pf3/select', // never use vanilla bootstrap-select
'@patternfly/patternfly': resolveModule('NONEXISTENT'),
'@patternfly/patternfly-next': resolveModule('NONEXISTENT'),
'@@MiQ-DDF': resolve(dirname(__filename), '../../app/javascript/forms/data-driven-form'),
Copy link
Contributor

Choose a reason for hiding this comment

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

what about just @@ddf

nobody will ever type MiQ with the right capitalization

Copy link
Contributor

Choose a reason for hiding this comment

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

And you may need a similar entry in jest.config.js - moduleNameMapper section

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

@skateman skateman force-pushed the expose-app-javascript branch from b6822b1 to 2e5ea11 Compare April 29, 2020 11:38
@miq-bot
Copy link
Member

miq-bot commented Apr 29, 2020

Checked commit skateman@5d870bf with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@skateman
Copy link
Member Author

skateman commented May 4, 2020

@himdel are you okay with these changes now?

@himdel
Copy link
Contributor

himdel commented May 18, 2020

Sorry, LGTM :)

@himdel himdel merged commit b1d8b18 into ManageIQ:master May 18, 2020
@skateman skateman deleted the expose-app-javascript branch May 19, 2020 05:29
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.

3 participants