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: Add decoration handler for handling fragment decoration #39

Merged
merged 7 commits into from
Aug 6, 2024

Conversation

FentPams
Copy link
Contributor

@FentPams FentPams commented Jul 30, 2024

Description

The fragment decoration needs more logic on handling decorations.
We leveraged Eventlistener, at the same time introduce decoration handler on index.js to handle the logic of redecorating.

reDecorateBlocks function and buildBlock fucntion are defined inside of project's script.js, where we need to pass from.

export function reDecorateBlocks(el) {
  if (!el.classList.contains('block')) return;
  decorateBlock(el);
  loadBlock(el);
}

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@FentPams
Copy link
Contributor Author

@ramboz I appreciate if you could let me know what's your thoughts on this :) I could improve accordingly and also add tests in. Thank you!

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated
@@ -709,6 +749,9 @@ async function runExperiment(document, pluginOptions) {
variant,
},
}));
if (variant !== 'control' && isRedecorateValid(this.buildBlock, this.reDecorateBlocks)) {
Copy link
Contributor

@ramboz ramboz Aug 1, 2024

Choose a reason for hiding this comment

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

If I'm not mistaken… page and section level will directly inline the HTML before the decoration as well, so the default Franklin decoration would work on those.

Only block/fragment done via the manifest are loaded after the decoration and would need to be redecorated. Correct?
So this could be moved to the ~L478 where we replace the HTML for the fragments.

We then can be in any of those use cases:

  1. we have a selector for an element inside a block and it doesn't need redecoration
  2. we have a selector for an element inside a block and the block needs to be redecorated
  3. we have a .block selector and we need to redecorate => switch block status to "loading" and call loadBlock(el)
  4. we have a .section selector and we need to redecorate => call decorateBlocks(el)
  5. we have a main selector and we need to redecorate => call decorateMain(el)
  6. we have a selector for an autoblock selector and we don't know what to do with it
  7. we have some random selector and we don't know what to do with it

We can offer a default implementation in scripts.js for 3, 4 & 5 pretty easily, but all the other ones will be dependent on the project implementation. We'd add this to the readme, since it's outside the scope of the plugin

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. Thanks! Makes more sense to move that in L478 :)

You are correct, the redecoration is only needed for fragment.

I did check if they are blocks in the project level(BHR case), if they are not, they will be ignored, so even adding that for page and section, it still works.

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (v2@c9ff2c0). Learn more about missing BASE report.

Additional details and impacted files
@@          Coverage Diff          @@
##             v2      #39   +/-   ##
=====================================
  Coverage      ?   93.42%           
=====================================
  Files         ?        1           
  Lines         ?      684           
  Branches      ?       11           
=====================================
  Hits          ?      639           
  Misses        ?       34           
  Partials      ?       11           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/index.js Outdated
@@ -436,6 +439,7 @@ function watchMutationsAndApplyFragments(
if (url && new URL(url, window.location.origin).pathname !== window.location.pathname) {
// eslint-disable-next-line no-await-in-loop
res = await replaceInner(new URL(url, window.location.origin).pathname, el, entry.selector);
pluginOptions.decorateFunction(el);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make that an await so we fully decorate before continuing.

I'd also update the README with the new property, and then we can merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you help review the updated README and see if it aligns with what you want? Thank you ☺️

@FentPams FentPams marked this pull request as ready for review August 2, 2024 20:01
@FentPams FentPams merged commit 5f942a2 into v2 Aug 6, 2024
5 checks passed
@FentPams FentPams deleted the decoration-handler branch August 6, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants