Implement rail style for selector #47

mfrisbey · 2023-08-21T23:24:10Z

Changes the style of the selector so that it appears as a rail on the right of the sidekick. In summary, the changes include the following:

Changes the asset selector's options so that it's more compact - this includes setting hideTreeNav and noWrap, which reduces the selector to the rail style, while not constraining us to drag/drop only.
Adds a "Copy" button to the Franklin integration's header, which will change its state based on selection events from the asset selector.
Clicking "Copy" will either:
- Use the existing method to load an <img /> tag into the clipboard (BTW: this way has never worked for me - I get a 500 from the WEB_TOOLS url request).
- If "Use Rapi" is defined in the block's config, the method will use the repository API to request a presigned URL for the asset blob, which it loads into the clipboard. Note that this will require a modified CORS config on the target AEM instance in order to work.
Misc cleanup, such as javadoc, copyrights, method renames, basic error handling, etc.

Test URLs:

aem-code-sync · 2023-08-21T23:24:12Z

Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

Re-run PSI Checks

aem-code-sync · 2023-08-21T23:24:28Z

Page	Scores	Audits	Google
/aem-asset-selector-stage

sdmcraft

Thanks @mfrisbey . Added a few comments.

sdmcraft · 2023-08-22T16:22:09Z

tools/sidekick/config.json

+      "paletteRect": "top: 50px; bottom: 10px; right: 10px; left: auto; width:400px; height: calc(100vh - 60px)"
+    },
+    {
+      "id": "asset-library-stage",


Do we need asset-library-stage ?

It's not required. This was more just for convenience for us, since we've been using a stage environment so that we can verify everything end-to-end, including using one of asset link's client IDs. You can see the document that corresponds with this button here.

FWIW, the stage button will only show up for anyone working on the asset selector code itself - it wouldn't need to be there for customers. But I can remove if and we can add the button locally if we don't want to include it here.

Yes probably its fine to have if it helps with testing. Customer are going to add their own sidekick configs and we'd document only the prod one.

sdmcraft · 2023-08-22T16:26:49Z

blocks/aem-asset-selector/aem-asset-selector-util.js

+    return maxRendition;
+  }
+  renditions
+    .filter((rendition) => rendition.type === 'image/png')


Is the rendition.type === 'image/png' check necessary? With it, we'd be omitting the better resolution renditions. I recollect having added it since clipboard api did not support other mime types. Do we still have that limitation?

The clipboard API only supports PNGs as blobs, so I added that for expedience. I'll add a TODO that we should revisit this and create a follow-up tickiet.

ok, I think the best png rendition we have is 319px which may become quite blurry on rendering. I thought the signed url from RAPI does not require auth headers but apparently it does need. One thing worth trying could be local object urls.

Wouldn't Franklin convert the images anyway into webp (or other formats/sizes depending on the js)? SO having PNG at least should be fine to start with an MVP.

Wouldn't Franklin convert the images anyway into webp

Yes it would but what I am not sure is whether it'd improve the image in anyway if it input image is a 319px png rendition. I agree that we can park it and tackle in a separate issue but overall seems to be an important requirement for real customer use cases.

Agreed that this needs to be improved. I put it in there as a "placeholder" without doing a great job communicating that it needs to be improved :)

I've created #48 as a follow-up to this.

sdmcraft · 2023-08-22T16:33:12Z

blocks/aem-asset-selector/aem-asset-selector-util.js

 const AS_MFE = 'https://experience.adobe.com/solutions/CQ-assets-selectors/static-assets/resources/assets-selectors.js';
 const IMS_ENV_STAGE = 'stg1';
 const IMS_ENV_PROD = 'prod';
 const API_KEY = 'franklin';
 const WEB_TOOLS = 'https://master-sacred-dove.ngrok-free.app';
+const REL_DOWNLOAD = 'http://ns.adobe.com/adobecloud/rel/download';
+const REL_RENDITIONS = 'http://ns.adobe.com/adobecloud/rel/rendition';
 // TODO: change this to Asset Link IMS client ID
 const IMS_CLIENT_ID = 'p66302-franklin';


Likely this should change to one of the pre-existing asset link client IDs.

Yes, we're planning on using one. I haven't changed it yet because we're waiting on a static host so that the client ID will work in production IMS.

Since we'll be defining a single block that everyone uses, we could potentially still require the client ID in the block config. That would at least keep the client ID out of a public github repository. It'll still end up in the rendered javascript, but at least it wouldn't be github that way.

aem-code-sync · 2023-08-22T17:23:23Z

Page	Scores	Audits	Google
/aem-asset-selector-stage

tmathern · 2023-08-22T17:44:02Z

blocks/aem-asset-selector/aem-asset-selector-util.js

+ * @param  {...any} theArgs Arguments to pass to the console log
+ *  statement.
+ */
+function logMessage(...theArgs) {


Do we really need a wrapper function here?

Not necessarily, I just added it so that we don't have to add an eslint ignore rule every time we write to the console.

tmathern · 2023-08-22T17:44:26Z

blocks/aem-asset-selector/aem-asset-selector-util.js

+ */
+function getRel(repositoryMetadata, rel) {
+  // eslint-disable-next-line no-underscore-dangle
+  return repositoryMetadata._links[rel];


Do we need to check for null/undefined to be more defensive?

tmathern · 2023-08-22T17:44:51Z

blocks/aem-asset-selector/aem-asset-selector-util.js

-    const json = await response.json();
-    return json['asset-url'];
+  if (!response) {
+    throw new Error('Did not receive response from request');


I prefer shorter messages

Suggested change

throw new Error('Did not receive response from request');

throw new Error('No response from request');

tmathern · 2023-08-22T17:45:36Z

blocks/aem-asset-selector/aem-asset-selector-util.js

-      maxRendition = rendition;
-    }
-  });
+  const renditions = getRel(asset, REL_RENDITIONS);


Either here or in getRel, we need to make sure asset is not null/undefined ("empty").

tmathern · 2023-08-22T17:46:30Z

blocks/aem-asset-selector/aem-asset-selector-util.js

+    return maxRendition;
+  }
+  renditions
+    .filter((rendition) => rendition.type === 'image/png')


Wouldn't Franklin convert the images anyway into webp (or other formats/sizes depending on the js)? SO having PNG at least should be fine to start with an MVP.

tmathern · 2023-08-22T17:48:39Z

blocks/aem-asset-selector/aem-asset-selector-util.js

+ * @returns {Promise} Resolves when the item has been written to the clipboard.
+ */
+async function copyToClipboardWithBinary(assetPublicUrl, mimeType) {
+  const binary = await fetch(assetPublicUrl);


Can we trust the URL to be properly formatted? Or should we verify it (so we can add nice error handling).

tmathern · 2023-08-22T17:53:11Z

blocks/aem-asset-selector/aem-asset-selector-util.js

+      return false;
+    }
+    const downloadJson = await res.json();
+    await copyToClipboardWithBinary(downloadJson.href, downloadJson.type);


I would proactively check that downloadJson is not null/undefined ("empty"), instead of possbly having it throw at the next line.

tmathern · 2023-08-22T17:53:52Z

blocks/aem-asset-selector/aem-asset-selector-util.js

+ * @returns {HTMLElement} The asset selector.
+ */
+function getAssetSelector() {
+  return document.getElementById('asset-selector');


I would make 'asset-selector' a constant on top an export it. I have a feeling this will be reused.

tmathern · 2023-08-22T17:54:40Z

blocks/aem-asset-selector/aem-asset-selector-util.js

+function handleAssetSelection(selection, cfg) {
+  if (cfg) {
+    if (selection.length && cfg.onAssetSelected) {
+      cfg.onAssetSelected(selection[0]);


If length > 1, can we log that we use only the first one? Or at least explain it in the readme.

tmathern · 2023-08-22T17:56:05Z

blocks/aem-asset-selector/aem-asset-selector.js

      <div id="login">
-        <p>Welcome to the Asset Selector! After signing in you'll have the option to select which assets to use.</p>
+        <p>Welcome to the Asset Selector! Please sign in to view your assets.</p>


Suggested change

<p>Welcome to the Asset Selector! Please sign in to view your assets.</p>

<p>Welcome to the Asset Selector. Please sign in to view your assets.</p>

tmathern · 2023-08-22T17:56:52Z

blocks/aem-asset-selector/aem-asset-selector.js

+      block.querySelector('#loading').style.display = 'none';
+      block.querySelector('#login').style.display = 'flex';
+    }
+  }, 2000);


Make the timeout value a constant, so we can reuse the same wait times for the UI/UX.

aem-code-sync · 2023-08-22T20:01:12Z

Page	Scores	Audits	Google
/aem-asset-selector-stage

aem-code-sync · 2023-08-22T20:06:22Z

Page	Scores	Audits	Google
/aem-asset-selector-stage

mfrisbey added 3 commits August 21, 2023 17:10

implement rail style UI for asset selector.

1d8e0ab

revert reintroduced proxy code.

123d6b2

fix lint errors

f8bfca2

mfrisbey requested review from sdmcraft, badvision and tmathern August 21, 2023 23:24

mfrisbey self-assigned this Aug 21, 2023

sdmcraft reviewed Aug 22, 2023

View reviewed changes

add note about png filter

7be25c1

tmathern reviewed Aug 22, 2023

View reviewed changes

tmathern approved these changes Aug 22, 2023

View reviewed changes

tmathern mentioned this pull request Aug 22, 2023

Rail integration #44

Closed

tmathern and others added 2 commits August 22, 2023 11:01

Update blocks/aem-asset-selector/aem-asset-selector-util.js

11c661d

make changes based on PR feedback

ce8c2cb

mfrisbey merged commit e291d79 into main Aug 22, 2023
2 checks passed

mfrisbey deleted the rail-selector branch August 22, 2023 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement rail style for selector #47

Implement rail style for selector #47

mfrisbey commented Aug 21, 2023

aem-code-sync bot commented Aug 21, 2023 •

edited

Loading

aem-code-sync bot commented Aug 21, 2023

sdmcraft left a comment

sdmcraft Aug 22, 2023

mfrisbey Aug 22, 2023

sdmcraft Aug 22, 2023

sdmcraft Aug 22, 2023

mfrisbey Aug 22, 2023

sdmcraft Aug 22, 2023

tmathern Aug 22, 2023

sdmcraft Aug 22, 2023 •

edited

Loading

mfrisbey Aug 22, 2023

sdmcraft Aug 22, 2023

mfrisbey Aug 22, 2023

aem-code-sync bot commented Aug 22, 2023

tmathern Aug 22, 2023

mfrisbey Aug 22, 2023

tmathern Aug 22, 2023 •

edited

Loading

tmathern Aug 22, 2023

tmathern Aug 22, 2023 •

edited

Loading

tmathern Aug 22, 2023

tmathern Aug 22, 2023

tmathern Aug 22, 2023

tmathern Aug 22, 2023

tmathern Aug 22, 2023

tmathern Aug 22, 2023

tmathern Aug 22, 2023

aem-code-sync bot commented Aug 22, 2023

aem-code-sync bot commented Aug 22, 2023

	throw new Error('Did not receive response from request');
	throw new Error('No response from request');

	<p>Welcome to the Asset Selector! Please sign in to view your assets.</p>
	<p>Welcome to the Asset Selector. Please sign in to view your assets.</p>

Implement rail style for selector #47

Implement rail style for selector #47

Conversation

mfrisbey commented Aug 21, 2023

aem-code-sync bot commented Aug 21, 2023 • edited Loading

aem-code-sync bot commented Aug 21, 2023

sdmcraft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdmcraft Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aem-code-sync bot commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmathern Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmathern Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aem-code-sync bot commented Aug 22, 2023

aem-code-sync bot commented Aug 22, 2023

aem-code-sync bot commented Aug 21, 2023 •

edited

Loading

sdmcraft Aug 22, 2023 •

edited

Loading

tmathern Aug 22, 2023 •

edited

Loading

tmathern Aug 22, 2023 •

edited

Loading