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

feat(lib): provide update and destroy methods on ECL.autoInit() - FRONT-33 #1544

Merged
merged 10 commits into from
Mar 13, 2020

Conversation

kalinchernev
Copy link
Contributor

@kalinchernev kalinchernev commented Feb 11, 2020

PR description

Intro

In order to provide new methods to return value of ECL.autoInit(), a breaking change is necessary. Current return type is an array, thus I am not sure it's making any sense to proxy methods like forEach.

@storybook/addon-options has been removed on purpose because it's not needed any more as after upgrade in #1520

Testing

Before testing, please reinstall dependencies and build presets to have the newly added methods into the ECL module.

yarn
yarn dist:presets
yarn start:ec

NB: do not remove yarn.lock file!

Tests can be done on https://5e426bdfbeb8426473e4cb3b--europa-component-library.netlify.com/playground/ec/?path=/story/templates-pages--main-policy-awareness
Changing between templates and optional items should not prevent file download expandable behavior to work.

To compare, go to https://ec.europa.eu/component-library/playground/ec/?path=/story/templates-pages--main-policy-awareness and toggle an optional element, try to click and expand file downloads.

papegaill
papegaill previously approved these changes Feb 11, 2020
Copy link
Contributor

@papegaill papegaill left a comment

Choose a reason for hiding this comment

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

Tested and working fine on my side. @emeryro do you want to give final approval on this?

@emeryro
Copy link
Contributor

emeryro commented Feb 19, 2020

The fix is working fine indeed, thanks for this
I am a little more concerned about the breaking change. We should clearly write a few lines (here or in the next release note) to explain what has to be done on integrators side.
Also, is the documentation still up to date concerning autoInit()? (on the component's api pages)

@kalinchernev
Copy link
Contributor Author

@papegaill @emeryro I agree that introducing a breaking change may cost more than what the existing approach provides.

If we decide to merge changes in the EC autoInit, maybe we'll also need to provide the same approach into the EU's autoinit, which is currently 1:1 copy of the same logic.

I've updated the most important part of the documentation https://github.com/ec-europa/europa-component-library/pull/1544/files#diff-1b0d268bd8fd3aa3e8f4fa27d6056c9a with notes on migration.

The scope of changes is only ECL's autoInit method. There are no changes with regards to how individual components instantiate/update/destroy their own JS behaviors.

@emeryro
Copy link
Contributor

emeryro commented Mar 3, 2020

We should unlock this PR.
I had a second look at it, and it seems that the changes are mostly on our side. The recommanded to use the component (using data-ecl-auto-init) won't be modified, right?
If that is the case, the impact is limited, and we could move forward and merge the PR

@kalinchernev
Copy link
Contributor Author

@emeryro is right, it'll be good to move this pull request forward regardless of whether we decide to merge it or not.

I made updates for the EU system in addition to previous work to make the PR ok to merge.

I also double-checked the most important implementations which we know of and they seem to not use per-component destroy methods which was my main concern.

In ec_theme doesn't seem to use the ECL.autoInit() is not used.

In oe_theme is using the auto init method, but destroy is not.

We will surely include release notes with links to the migration guides added in the PR to mitigate potential issues as much as possible.

@emeryro emeryro removed their assignment Mar 13, 2020
@emeryro emeryro merged commit 9326f89 into v2-dev Mar 13, 2020
@emeryro emeryro deleted the fix/FRONT-33 branch March 13, 2020 12:51
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.

None yet

3 participants