GH-244 - Extensible source support #245

auniverseaway · 2024-09-06T19:45:55Z

Description

Provides ability to let project spec an editUrlFormat object
Supports {{contentSourceUrl}} {{org}} {{site}} {{pathname}} placeholders in pattern

API

UE

{
  "project": "XWalk Block Collection",
  "editUrlLabel": "Universal Editor",
  "editUrlPattern": "{{contentSourceUrl}}{{pathname}}?cmd=open"
}

DA

{
  "project": "DA Block Collection",
  "editUrlLabel": "Document Authoring",
  "editUrlPattern": "https://da.live/edit#/{{org}}/{{site}}{{pathname}}"
}

Related Issue

Resolves: GH-244

Motivation and Context

See the GH issue.

How Has This Been Tested?

E2E for now. Once approach is agreed upon, will write tests.

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.

auniverseaway · 2024-09-06T19:47:39Z

I tried to strike a balance between changing very deep parts of Sidekick / Admin and simply bolting on something that cannot be maintained.

My hope is that this can work for XWalk / UE as well.

src/extension/app/store/app.js

dylandepass · 2024-09-06T20:54:32Z

Supporting this makes sense to me, and I don’t see any immediate issues with this approach. However, @rofe might have a different perspective. It would also be good to get input from UE to confirm if this works for them as you suggested.

One suggestion: instead of sourceFormat, perhaps something like contentSourceEditFormat would be more consistent with related fields like contentSourceUrl and contentSourceType.

auniverseaway · 2024-09-07T00:29:15Z

One suggestion: instead of sourceFormat, perhaps something like contentSourceEditFormat would be more consistent with related fields like contentSourceUrl and contentSourceType.

I changed things to get closer to your comment above.

A couple notes:

The patterns exposed in sidekick/config.json are less verbose, so I did keep sourceFormat as the entry for a project to try and keep similar short conventions..
Putting two top-level properties in sidekick/config.json seemed heavy, so I did make sourceFormat an object with two properties.
Since contentSourceType already exists, and is markup in these scenarios, I was reluctant to change that property to what is provided by the project. I see markup being useful in the long run. There would also be casing issues like the existing onedrive > SharePoint that is in getContentSourceLabel.
This PR introduces contentSourceLabel as the text that is used in the button. SP & GDrive button labels by comparison are calculated based on other details (fstab, etc.) then matched to their title cased equivalents.
This PR introduces contentSourcePattern to better match the naming conventions of the codebase.
While the sidekick/config.json is an object with sub-props, I do flatten them down to better fit the style of the codebase. This makes null checks a bit cleaner, too.

Let me know what y'all think. Happy to adjust further.

The final sidekick/config.json format looks like:

{
  "project": "DA Block Collection",
  "sourceFormat": {
    "label": "Document Authoring",
    "pattern": "https://da.live/edit#/{{org}}/{{repo}}{{pathname}}"
  }
}

rofe

Looks good in general, but I'm a little concerned that this will be a one-off solution since I'm not sure any of the other BYOM sources will be able provide a pattern containing org and site...

rofe · 2024-09-07T14:02:19Z

src/extension/app/store/site.js

@@ -247,6 +247,7 @@ export class SiteStore {
      specialViews,
      transient = false,
      scriptUrl = 'https://www.hlx.live/tools/sidekick/index.js',
+      sourceFormat,


I think editUrlFormat would be more descriptive.

rofe · 2024-09-07T14:02:33Z

src/extension/app/store/app.js

+    if (!contentSourcePattern || typeof contentSourcePattern !== 'string') return null;
+    const url = contentSourcePattern
+      .replace('{{org}}', owner)
+      .replace('{{repo}}', repo)


I would consistently switch from owner/repo to org/site in all public facing API surfaces.

Suggested change

.replace('{{repo}}', repo)

.replace('{{site}}', repo)

rofe · 2024-09-07T14:11:52Z

My hope is that this can work for XWalk / UE as well.

I looked at some existing xwalk mountpoints and it looks like the pattern could be fulfilled. Letting @buuhuu confirm.

rofe · 2024-09-08T13:24:50Z

Wondering if the label ever need to be localized? If so, you could add an additional, optional labelI18n object, analog to the existing titleI18n allowing localization of custom button texts in the current schema.

mhaack · 2024-09-09T09:21:30Z

@rofe @auniverseaway and I chatted about this on Slack, I think this will work with XWalk as well. Today we have a Sidekick overlay which just does

const { pathname } = window.location;
const editorUrl = `${contentSourceUrl}${pathname}?cmd=open`;

In the new format this would be

{
  "project": "abc",
  "sourceFormat": {
    "label": "Universal Editor",
    "pattern": "https://author-p1234-e5678.adobeaemcloud.com/bin/franklin.delivery/{{org}}/{{repo}}/main/{{pathname}}?cmd=open"
  }
}

It will require us to put the content source URL here a second time, which I think is similar to DA was well. But give this is typically one time config it might be fine. Alternative would be to have a {{contentSource}} variable as well.

buuhuu · 2024-09-09T10:53:26Z

We should not duplicate the {{contentSourceUrl}} if possible, same as we should not duplicate owner, repo, branch. Either we replace all patterns or none. If we say it is fine to duplicate these parameters in the config the proposal would even work without any parameter replacement iiuc.

auniverseaway · 2024-09-09T16:30:13Z

@buuhuu,
If @rofe approves and it works for you, I think tokenizing {{contentSourceUrl}} makes sense. The goal is to put this property in the respective boilerplates and you don't have to change it project to project.

For an XWalk project, I show:

contentSourceUrl

https://author-p15404-e146221-cmstg.adobeaemcloud.com/bin/franklin.delivery/mhaack/my-aem-project/main

Edit URL

https://author-p15404-e146221-cmstg.adobeaemcloud.com/bin/franklin.delivery/mhaack/my-aem-project/main/index?cmd=open

Which means that should work well for UE.

DA / WordPress / Drupal / etc have different content and edit URLs.

buuhuu · 2024-09-09T17:40:08Z

The goal is to put this property in the respective boilerplates and you don't have to change it project to project

Can you elaborate on this? With hlx5 repo-less it cannot be in the boilerplate / it changes from project to project.

rofe · 2024-09-09T18:20:50Z

@buuhuu the reason for supporting variables is that this edit url format can stay fully generic and doesn't need to be changed from project to project.

@auniverseaway I approve :)

auniverseaway · 2024-09-09T21:03:43Z

I think I accounted for all feedback and suggestions.

@rofe the only thing I didn't add was i18n for the label as it didn't seem like proper nouns would need to be localized: SharePoint, Google Drive, Universal Editor, WordPress, Document Authoring (as a brand name), etc. If you feel strongly we should add the support, let me know.

I modified the original comment with the updated spec, but here it is as well. If I can get thumbs on the final naming conventions I can work on tests and docs.

Thanks!

Sidekick spec

UE

{
  "project": "XWalk Block Collection",
  "editUrlFormat": {
    "label": "Universal Editor",
    "pattern": "{{contentSourceUrl}}{{pathname}}?cmd=open"
  }
}

DA

{
  "project": "DA Block Collection",
  "editUrlFormat": {
    "label": "Document Authoring",
    "pattern": "https://da.live/edit#/{{org}}/{{site}}{{pathname}}"
  }
}

rofe · 2024-09-10T09:12:21Z

@auniverseaway we'd also need to update the config schema. That makes me wonder if it wouldn't be easier to just add 2 top level properies instead of an object?

mhaack · 2024-09-10T15:09:31Z

Tested it with https://main--my-aem-project--mhaack.aem.page/ for UE & Crosswalks, works nicely. cc @buuhuu

auniverseaway · 2024-09-10T15:09:46Z

@rofe I saw that as I was writing tests and getting yelled at by TypeScript. I'll make them both top level.

dylandepass · 2024-09-11T14:17:14Z

@rofe I saw that as I was writing tests and getting yelled at by TypeScript. I'll make them both top level.

@auniverseaway You'll want to add them to ClientConfig typedef to get the type checker to stop complaining.

* Provides ability to let project spec an `editUrlFormat` object * Supports `{{contentSourceUrl}} {{org}} {{site}} {{pathname}}` placeholders in pattern

auniverseaway · 2024-09-18T21:07:09Z

@rofe @dylandepass I think I've got all suggestions covered. I didn't see the schema in the codebase, so my assumption is that it's generated from the TS def?

rofe · 2024-09-19T13:04:48Z

I didn't see the schema in the codebase

At the moment, the schema needs to be updated manually here:
https://github.com/adobe/helix-website/blob/main/tools/sidekick/config.schema.json

auniverseaway · 2024-09-20T04:53:20Z

@rofe updated: adobe/helix-website#657

rofe · 2024-09-20T07:57:32Z

@auniverseaway I'll merge this into a branch to make sure all checks are passing before mergin to main.

This reverts commit 5292c5f.

Co-authored-by: Chris Millar <[email protected]>

# [1.32.0](v1.31.4...v1.32.0) (2024-09-24) ### Bug Fixes * dynamically load in ui.js ([a4fc57d](a4fc57d)) * remove dynamic import from background script ([58cc888](58cc888)) ### Features * extensible source support ([#245](#245)) ([#268](#268)) ([a834be2](a834be2)) * **publish:** add confirm option ([#262](#262)) ([7e8e1fc](7e8e1fc)) * support transient site token ([#272](#272)) ([2aeb56a](2aeb56a))

auniverseaway requested review from rofe and dylandepass September 6, 2024 19:45

dylandepass reviewed Sep 6, 2024

View reviewed changes

src/extension/app/store/app.js Outdated Show resolved Hide resolved

auniverseaway force-pushed the gh244-byomsource branch from 39e6ad5 to 7c0e0c2 Compare September 6, 2024 21:28

rofe requested changes Sep 7, 2024

View reviewed changes

auniverseaway force-pushed the gh244-byomsource branch from 7c0e0c2 to 524b0ad Compare September 9, 2024 20:33

adobeGH-244 - Extensible source support

c27170c

* Provides ability to let project spec an `editUrlFormat` object * Supports `{{contentSourceUrl}} {{org}} {{site}} {{pathname}}` placeholders in pattern

auniverseaway force-pushed the gh244-byomsource branch from 524b0ad to c27170c Compare September 18, 2024 21:01

auniverseaway marked this pull request as ready for review September 18, 2024 21:02

auniverseaway requested review from rofe and dylandepass September 18, 2024 21:04

rofe approved these changes Sep 19, 2024

View reviewed changes

rofe changed the base branch from main to issue-244 September 20, 2024 07:59

rofe merged commit 5292c5f into adobe:issue-244 Sep 20, 2024
2 checks passed

rofe pushed a commit that referenced this pull request Sep 20, 2024

GH-244 - Extensible source support (#245)

a2e8780

rofe mentioned this pull request Sep 20, 2024

GH-244 - Extensible source support (#245) #268

Merged

rofe added a commit that referenced this pull request Sep 20, 2024

Revert "GH-244 - Extensible source support (#245)"

de1a868

This reverts commit 5292c5f.

rofe added a commit that referenced this pull request Sep 24, 2024

feat: extensible source support (#245) (#268)

a834be2

Co-authored-by: Chris Millar <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-244 - Extensible source support #245

GH-244 - Extensible source support #245

auniverseaway commented Sep 6, 2024 •

edited

Loading

auniverseaway commented Sep 6, 2024

dylandepass commented Sep 6, 2024 •

edited

Loading

auniverseaway commented Sep 7, 2024 •

edited

Loading

rofe left a comment

rofe Sep 7, 2024 •

edited

Loading

rofe Sep 7, 2024 •

edited

Loading

rofe commented Sep 7, 2024

rofe commented Sep 8, 2024

mhaack commented Sep 9, 2024 •

edited

Loading

buuhuu commented Sep 9, 2024

auniverseaway commented Sep 9, 2024

buuhuu commented Sep 9, 2024

rofe commented Sep 9, 2024

auniverseaway commented Sep 9, 2024

rofe commented Sep 10, 2024

mhaack commented Sep 10, 2024

auniverseaway commented Sep 10, 2024

dylandepass commented Sep 11, 2024 •

edited

Loading

auniverseaway commented Sep 18, 2024

rofe commented Sep 19, 2024

auniverseaway commented Sep 20, 2024

rofe commented Sep 20, 2024

GH-244 - Extensible source support #245

GH-244 - Extensible source support #245

Conversation

auniverseaway commented Sep 6, 2024 • edited Loading

Description

API

UE

DA

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

auniverseaway commented Sep 6, 2024

dylandepass commented Sep 6, 2024 • edited Loading

auniverseaway commented Sep 7, 2024 • edited Loading

rofe left a comment

Choose a reason for hiding this comment

rofe Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

rofe Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

rofe commented Sep 7, 2024

rofe commented Sep 8, 2024

mhaack commented Sep 9, 2024 • edited Loading

buuhuu commented Sep 9, 2024

auniverseaway commented Sep 9, 2024

For an XWalk project, I show:

contentSourceUrl

Edit URL

buuhuu commented Sep 9, 2024

rofe commented Sep 9, 2024

auniverseaway commented Sep 9, 2024

Sidekick spec

UE

DA

rofe commented Sep 10, 2024

mhaack commented Sep 10, 2024

auniverseaway commented Sep 10, 2024

dylandepass commented Sep 11, 2024 • edited Loading

auniverseaway commented Sep 18, 2024

rofe commented Sep 19, 2024

auniverseaway commented Sep 20, 2024

rofe commented Sep 20, 2024

auniverseaway commented Sep 6, 2024 •

edited

Loading

dylandepass commented Sep 6, 2024 •

edited

Loading

auniverseaway commented Sep 7, 2024 •

edited

Loading

rofe Sep 7, 2024 •

edited

Loading

rofe Sep 7, 2024 •

edited

Loading

mhaack commented Sep 9, 2024 •

edited

Loading

dylandepass commented Sep 11, 2024 •

edited

Loading