-
Notifications
You must be signed in to change notification settings - Fork 626
Pro 8152 anchors #5071
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
Pro 8152 anchors #5071
Changes from 5 commits
b2f369d
2b0f0da
e388e0a
0dd7caa
f8286b8
fa937e9
c5501e9
dc70b1e
0d03767
5524b4f
3f7ca89
4bf5bab
730219e
cae8b19
17a3f3f
5905c61
bc93cab
04c9a8c
ce69f46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -432,8 +432,8 @@ module.exports = { | |
| const parse = config.parse | ||
| ? config.parse | ||
| : function (parser, nodes, lexer) { | ||
| // Default parser gets comma separated arguments, | ||
| // assumes no body | ||
| // Default parser gets comma separated arguments, | ||
| // assumes no body | ||
|
|
||
| // get the tag token | ||
| const token = parser.nextToken(); | ||
|
|
@@ -696,7 +696,7 @@ module.exports = { | |
| _.extend(args, data); | ||
|
|
||
| if (req.aposError) { | ||
| // A 500-worthy error occurred already, i.e. in `pageBeforeSend` | ||
| // A 500-worthy error occurred already, i.e. in `pageBeforeSend` | ||
| telemetry.handleError(span, req.aposError); | ||
| span.end(); | ||
| return error(req.aposError); | ||
|
|
@@ -739,8 +739,8 @@ module.exports = { | |
| span.setStatus({ code: telemetry.api.SpanStatusCode.OK }); | ||
| return content; | ||
| } catch (e) { | ||
| // The page template threw an exception. Log where it | ||
| // occurred for easier debugging | ||
| // The page template threw an exception. Log where it | ||
| // occurred for easier debugging | ||
| telemetry.handleError(span, e); | ||
| return error(e); | ||
| } finally { | ||
|
|
@@ -1289,7 +1289,7 @@ module.exports = { | |
| // and `choices` properties, and guaranteeing that `items` exists, | ||
| // at least as an empty array. | ||
|
|
||
| annotateAreaForExternalFront(field, area) { | ||
| async annotateAreaForExternalFront(field, area) { | ||
| area.field = field; | ||
| area.options = field.options; | ||
| // Really widget configurations, but the method name is already set in | ||
|
|
@@ -1303,11 +1303,27 @@ module.exports = { | |
| label: options.addLabel || manager.label || `No label for ${name}` | ||
| }; | ||
| }).filter(choice => !!choice); | ||
|
|
||
| area.items ||= []; | ||
| if (area._docId) { | ||
| for (const item of area.items) { | ||
| for (const item of area.items) { | ||
| // Add _docId if area has one | ||
| if (area._docId) { | ||
| item._docId = area._docId; | ||
| } | ||
|
|
||
| // Annotate each individual widget with its options | ||
| // Each widget must elect into this by creating an | ||
| // `annotateWidgetForExternalFront() method. | ||
| const manager = self.apos.area.getManager?.(item.type) || | ||
| self.apos.area.widgetManagers?.[item.type]; | ||
|
|
||
| let widgetOptions = {}; | ||
| if (manager && typeof manager.annotateWidgetForExternalFront === 'function') { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need this if anymore. It's going to exist.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! |
||
| widgetOptions = await manager.annotateWidgetForExternalFront() || {}; | ||
| }; | ||
| if (widgetOptions && Object.keys(widgetOptions).length > 0) { | ||
| item._options = widgetOptions; | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -449,7 +449,12 @@ module.exports = { | |
| } | ||
| return true; | ||
| }); | ||
| }, | ||
|
|
||
| annotateWidgetForExternalFront() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
| return {}; | ||
| } | ||
|
|
||
| }; | ||
| }, | ||
| extendMethods(self) { | ||
|
|
||
There was a problem hiding this comment.
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.