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

Fix article content profile switch in authoring #4684

Merged
merged 8 commits into from
Nov 13, 2024

Conversation

thecalcc
Copy link
Contributor

@thecalcc thecalcc commented Nov 11, 2024

SDESK-7417

@@ -359,6 +359,22 @@ export class AuthoringIntegrationWrapper extends React.PureComponent<IPropsWrapp
},
retrieveStoredValue: (item: IArticle, fieldId) => item.extra?.[fieldId] ?? null,
}}
customHeader={((options) => (
Copy link
Member

Choose a reason for hiding this comment

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

use exposed to keep naming consistent

@@ -215,6 +216,8 @@ declare module 'superdesk-api' {
reinitialize(itemWithChanges: T): void;
}>>;

customHeader?: (options: IExposedFromAuthoring<T>) => JSX.Element;
Copy link
Member

Choose a reason for hiding this comment

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

Return Array<ITopBarWidget<T>> here. We already have a nice interface that supports ordering buttons, adding keybindings. This is just another toolbar and it'd be best if it was configurable the same way.

@@ -161,6 +161,7 @@ declare module 'superdesk-api' {
initiateClosing(): void;
keepChangesAndClose(): void;
stealLock(): void;
onChangeToolbarWidget(item: T): void;
Copy link
Member

Choose a reason for hiding this comment

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

Naming doesn't make sense for me here. What is a toolbar widget? What does it mean for a toolbar widget to "change"? from widget A to widget B? or how? Why would then the article be passed if it was only the toolbar widget that changed?

I think you should simply expose reinitialize: (itemWithChanges: IArticle) => void; that is already exposed in multiple places in the API, just add a profile argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't because state needs to be passed, so some wrapper would always have to be there

Copy link
Member

Choose a reason for hiding this comment

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

remove onChangeToolbarWidget completely since there are no usages anymore

@tomaskikutis
Copy link
Member

fix ci

@@ -161,6 +161,7 @@ declare module 'superdesk-api' {
initiateClosing(): void;
keepChangesAndClose(): void;
stealLock(): void;
onChangeToolbarWidget(item: T): void;
Copy link
Member

Choose a reason for hiding this comment

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

remove onChangeToolbarWidget completely since there are no usages anymore

@@ -215,6 +216,8 @@ declare module 'superdesk-api' {
reinitialize(itemWithChanges: T): void;
}>>;

customHeader?: (options: IExposedFromAuthoring<T>) => Array<ITopBarWidget<T>>;
Copy link
Member

Choose a reason for hiding this comment

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

headerToolbar would be a better name.

<ContentProfileDropdown
item={entity}
reinitialize={(item) => {
if (exposed.hasUnsavedChanges()) {
Copy link
Member

Choose a reason for hiding this comment

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

It'd be cleaner if getProfileAndReinitialize would only be used once. This way it is also clear that it will get called regardless of the condition.

const handledChanges = exposed.hasUnsavedChanges()
    ? exposed.handleUnsavedChanges()
    : Promise.resolve();

handledChanges.then(() => {
    getProfileAndReinitialize(item);
});

@@ -1220,6 +1219,7 @@ export class AuthoringReact<T extends IBaseRestApiResponse> extends React.PureCo
pinnedId: id === activeWidgetId ? activeWidgetId : this.props.sideWidget?.pinnedId,
});
},
reinitialize: (item, profile) => this.reinitialize(state, item, profile),
Copy link
Member

Choose a reason for hiding this comment

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

you didn't add reinitialize to the interface.

Copy link
Member

@tomaskikutis tomaskikutis left a comment

Choose a reason for hiding this comment

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

fix ci

@thecalcc thecalcc enabled auto-merge (squash) November 13, 2024 12:17
@thecalcc thecalcc merged commit 2c1f3ac into superdesk:develop Nov 13, 2024
3 checks passed
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