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

Update with new superdesk content profile API #2089

Merged
merged 8 commits into from
Sep 17, 2024

Conversation

thecalcc
Copy link
Contributor

@thecalcc thecalcc commented Sep 16, 2024

SDCP-838

Front-end checklist

  • This pull request is adding missing TypeScript types to modified code segments where it's easy to do so with confidence
  • This pull request is using TypeScript interfaces instead of prop-types and updates usages where it's quick to do so
  • This pull request is using memo or PureComponent to define new React components (and updates existing usages in modified code segments)
  • This pull request is replacing lodash.get with optional chaining and nullish coalescing for modified code segments
  • This pull request is not importing anything from client-core directly (use superdeskApi)
  • This pull request is importing UI components from superdesk-ui-framework and superdeskApi when possible instead of using ones defined in this repository.
  • This pull request is not using planningApi where it is possible to use superdeskApi
  • This pull request is not adding redux based modals
  • In this pull request, properties of redux state are not being passed as props to components; instead, we connect it to the component that needs them. Except components where using a react key is required - do not connect those due to performance reasons.
  • This pull request is not adding redux actions that do not modify state (e.g. only calling angular services; those should be moved to planningApi)

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.

Go over the checklist and mark the ones that apply. It might not be possible to implement some, in that case leave unchecked.

@@ -20,6 +21,24 @@ const DEFAULT_PLANNING_SCHEMA = {
};

export class AddToPlanningController {
$scope: any;
notify: typeof superdeskApi['ui']['notify'];
Copy link
Member

Choose a reason for hiding this comment

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

It's not correct to specify this type. You should not make assumptions on what superdesk API is using under the hood as implementation. It should be possible to easily change it without breaking anything.

@tomaskikutis
Copy link
Member

Regarding checklist, if it doesn't apply to the PR (i.e. no lodash usages), mark as done. I'd like to quickly see whether there are any guidelines that do apply, but were not implemented.

@thecalcc
Copy link
Contributor Author

thecalcc commented Sep 17, 2024

Regarding checklist, if it doesn't apply to the PR (i.e. no lodash usages), mark as done. I'd like to quickly see whether there are any guidelines that do apply, but were not implemented.

image

I went over the checklist, and I checked all the boxes that are done by me, thus apply to the PR

image

.then((newsItem: IArticle) => {
const contentProfile = superdeskApi.entities.contentProfile.get(newsItem.profile);

return Promise.resolve({
Copy link
Member

Choose a reason for hiding this comment

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

no need for Promise.resolve here. You should return the object directly. Since it's inside a .then it's a promise already

@thecalcc thecalcc merged commit a7d1a96 into superdesk:develop Sep 17, 2024
12 checks passed
thecalcc added a commit to thecalcc/superdesk-planning that referenced this pull request Sep 17, 2024
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