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

Feat coverage profiles #2217

Merged
merged 23 commits into from
Mar 11, 2025
Merged

Conversation

thecalcc
Copy link
Contributor

@thecalcc thecalcc commented Feb 27, 2025

STT-111

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)

@thecalcc thecalcc changed the base branch from develop to feature/embedded-events-editor February 27, 2025 11:06
@thecalcc thecalcc mentioned this pull request Feb 28, 2025
10 tasks
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.

While the task requires being able to configure a distinct content profile for each content type e.g. text, video, audio - the implementation itself at the database level or at data-structure level - should not be coupled to content type. If a new requirement came in tomorrow - that we want multiple profiles for a single content type - that shouldn't require us to do changes to database structure or data structures. It should be enough to adjust the UI in the modal.

I remember discussing this - that we'd retrieve profiles by ID - not content type with @petrjasek. Not sure if you @thecalcc wasn't participating in the discussion or you guys changed the approach.

field: getFieldNameTranslated(fields[0]).toUpperCase()
}));
} else {
notify.error(gettext('At least one "{{fields}}" fields are required by the system', {
Copy link
Member

Choose a reason for hiding this comment

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

field {{name}} is required by the system would be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this code was actually kind of bad. I began reworking it, but it was tied to other bad code, then I realized that some stuff related to embeddedProfile was no longer used now that I decoupled the coverage profiles. So I now have reworked it to work without this case, while achieving the same functionality in a better and more obvious way for the user - by notifying them about each required field

@thecalcc
Copy link
Contributor Author

thecalcc commented Mar 7, 2025

While the task requires being able to configure a distinct content profile for each content type e.g. text, video, audio - the implementation itself at the database level or at data-structure level - should not be coupled to content type. If a new requirement came in tomorrow - that we want multiple profiles for a single content type - that shouldn't require us to do changes to database structure or data structures. It should be enough to adjust the UI in the modal.

I remember discussing this - that we'd retrieve profiles by ID - not content type with @petrjasek. Not sure if you @thecalcc wasn't participating in the discussion or you guys changed the approach.

It's not actually coupled to the content_type I think. Now we use that as just a property right @petrjasek ? We can always make the creation of new coverage types also dynamic which would pass on the backend because in the schema it's defined like this: "content_type": {"type": "string", "required": True},. Then we can adjust frontend according to the requirements - instead of working with a static type we'll use ids and list all coverage profiles by fetching them all. Just like we do with article profiles probably. For now how I understand it - we should list profiles for all coverage types, regardless whether they were actually created by the user or not - if not - we render the old profile. If it was created after this update - we show the new data.

@thecalcc thecalcc requested a review from tomaskikutis March 7, 2025 11:26
@thecalcc
Copy link
Contributor Author

thecalcc commented Mar 7, 2025

To add to my comment - now I think we should keep things as they are in the UI. Otherwise we need to either make some workaround (mock some static profiles so users can access the in the profile config) to prepare for a feature that may never come. And when it comes we can add more UI - a plus button to add coverage profiles dynamically, and endless content_types. And a text input for content_type field itself and then fetch everything by id.

@petrjasek
Copy link
Member

petrjasek commented Mar 7, 2025

While the task requires being able to configure a distinct content profile for each content type e.g. text, video, audio - the implementation itself at the database level or at data-structure level - should not be coupled to content type. If a new requirement came in tomorrow - that we want multiple profiles for a single content type - that shouldn't require us to do changes to database structure or data structures. It should be enough to adjust the UI in the modal.

I remember discussing this - that we'd retrieve profiles by ID - not content type with @petrjasek. Not sure if you @thecalcc wasn't participating in the discussion or you guys changed the approach.

we're ready for multiple on the backend, nothing changed in that regard

@tomaskikutis
Copy link
Member

image

@dzonidoo could you improve this PR to make button content left-aligned?

See client/components/ContentProfiles/CoverageProfileModal.tsx.
You'll need to add that capability to ui-framework buttons. Use RTL friendly names for the prop
textAlign: 'start' | 'end' | 'center' // defaults to 'center'

@petrjasek petrjasek merged commit 653bdbf into feature/embedded-events-editor Mar 11, 2025
20 checks passed
@petrjasek petrjasek deleted the feat-coverage-profiles branch March 11, 2025 11:36
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