Skip to content

Pro 8152 anchors#5071

Closed
BoDonkey wants to merge 19 commits intomainfrom
pro-8152-anchors
Closed

Pro 8152 anchors#5071
BoDonkey wants to merge 19 commits intomainfrom
pro-8152-anchors

Conversation

@BoDonkey
Copy link
Copy Markdown
Contributor

Summary

Summarize the changes briefly, including which issue/ticket this resolves. If it closes an existing Github issue, include "Closes #[issue number]"
This PR updates the annotateAreaForExternalFront() method to accept annotateWidgetForExternalFront() methods from any widget module. This will allow for individual widgets to pass options to an external frontend, like Astro.

What are the specific steps to test this change?

For example:

  1. Run the website and log in as an admin
  2. Open a piece manager modal and select several pieces
  3. Click the "Archive" button on the top left of the manager and confirm that it should proceed
  4. Check that all pieces have been archived properly
  1. In the starter-kit-astro-essentials backend add a project-level option to the image-widget.
  2. In the frontend, modify the image widget to log the options from the frontmatter.
  3. Add an image widget to any area and check the logs.

What kind of change does this PR introduce?

(Check at least one)

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Build-related changes
  • Other

Make sure the PR fulfills these requirements:

  • It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
  • The changelog is updated
  • Related documentation has been updated
  • Related tests have been updated

If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

@BoDonkey BoDonkey requested a review from boutell September 11, 2025 14:07
@linear
Copy link
Copy Markdown

linear Bot commented Sep 11, 2025

Copy link
Copy Markdown
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

Use area.getManager.

Provide an empty stub method in the base class of widgets so you don't have to do an if test and it is more self-documenting for widget developers. Also important to establish the pattern so that people can use extendMethods which would be thwarted if they had to check if it existed first.

In general though, this is great.

@BoDonkey
Copy link
Copy Markdown
Contributor Author

Took me a minute, but I think this is what you want.

@BoDonkey BoDonkey requested a review from boutell September 12, 2025 18:29
Copy link
Copy Markdown
Member

@boutell boutell left a comment

Choose a reason for hiding this comment

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

One small thing 🙏

self.apos.area.widgetManagers?.[item.type];

let widgetOptions = {};
if (manager && typeof manager.annotateWidgetForExternalFront === 'function') {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You don't need this if anymore. It's going to exist.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By adding it to the base class we've made it part of the contract you can expect.

I guess you can still do if (manager).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought I would have to error on the side of caution. Learning!

});
},

annotateWidgetForExternalFront() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@BoDonkey BoDonkey requested a review from boutell September 12, 2025 19:19
};
if (widgetOptions && Object.keys(widgetOptions).length > 0) {
const widgetOptions = await manager.annotateWidgetForExternalFront() || {};
if (Object.keys(widgetOptions).length > 0) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not always provide it, so people on the astro side don't have to drive defensively with ?.? An empty object isn't that expensive.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated

@BoDonkey BoDonkey requested a review from boutell September 15, 2025 12:37
// at least as an empty array.

annotateAreaForExternalFront(field, area) {
async annotateAreaForExternalFront(field, area) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this really need to be async? It's a major performance hit when multiplied by every area and then every widget, and there's no async work being done to look up options. Also I don't think you changed anything where it's called, so there's probably just a promise showing up there?

My starting assumption is that this annotation work should not be async. It's meant to be very lightweight to minimize the impact of using an external frontend.

const manager = self.apos.area.getManager?.(item.type) ||
self.apos.area.widgetManagers?.[item.type];

const widgetOptions = await manager.annotateWidgetForExternalFront() || {};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should not be async either.

});
},

annotateWidgetForExternalFront() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Noticing this is already not async (and that's good), I think "async" in the area annotation method might have been a discarded early thought that hung around?

@boutell
Copy link
Copy Markdown
Member

boutell commented Sep 15, 2025

Based on the way "async" was hanging around in that method and "await" was being used even though it was called synchronously, my impression is that this probably hasn't been tested yet in an Astro project?

@BoDonkey
Copy link
Copy Markdown
Contributor Author

After a bit of nonsense with getting apostrophe linked in, I removed the erroneous asyncs and confirmed that everything was functional in the starter-kit-astro-essentials with the improved @apostrophecms/anchors. All options now passed to the external front.

@BoDonkey BoDonkey requested a review from boutell September 15, 2025 15:28
boutell
boutell previously approved these changes Sep 16, 2025
Comment thread CHANGELOG.md

### Fixes

* Resolve inline image URLs correctly when in edit mode and not in the default locale.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like one of Miro's PRs got caught up in this PR somehow. But I'm going to ignore it because I know I approved it.

@BoDonkey BoDonkey dismissed boutell’s stale review September 16, 2025 13:39

The merge-base changed after approval.

@BoDonkey BoDonkey requested a review from boutell September 21, 2025 09:44
boutell
boutell previously approved these changes Sep 22, 2025
@BoDonkey BoDonkey dismissed boutell’s stale review September 22, 2025 13:17

The merge-base changed after approval.

@BoDonkey BoDonkey closed this Sep 22, 2025
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.

3 participants