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

Feature: Checklist #5408

Merged
merged 21 commits into from
Aug 27, 2024
Merged

Feature: Checklist #5408

merged 21 commits into from
Aug 27, 2024

Conversation

pmattmann
Copy link
Member

@pmattmann pmattmann commented Jun 20, 2024

  • Merge this PR
  • Frontend:
    • Administration: Manage checklists
    • ChecklistNode

I would merge the backend early so that the two frontend components can be developed at the same time.

Relates to #4936

@pmattmann pmattmann marked this pull request as ready for review June 21, 2024 20:10
Copy link
Member

@manuelmeister manuelmeister left a comment

Choose a reason for hiding this comment

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

Currently while prototyping the frontend (#5460), I thought to myself if we probably need an (optional) numbering system for the checklist items, similar to the schedule entries? What do you think?

While trying to recreate the PBS checklist, I realized that 64 characters are not enough for checklist item texts, or then we need an additional description for longer texts.

@pmattmann
Copy link
Member Author

Currently while prototyping the frontend (#5460), I thought to myself if we probably need an (optional) numbering system for the checklist items, similar to the schedule entries? What do you think?

Currently this is not a requirement.
Shouldn't this question be discussed here #4936?
We will no longer consider the PR after the merge.

While trying to recreate the PBS checklist, I realized that 64 characters are not enough for checklist item texts, or then we need an additional description for longer texts.

I am happy to adapt this. How many characters should be supported?

Copy link
Contributor

@BacLuc BacLuc left a comment

Choose a reason for hiding this comment

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

Very cool, thank you.
Some technicalities, most of which we could tackle later

api/fixtures/checklistItems.yml Outdated Show resolved Hide resolved
api/migrations/schema/Version20240620143000.php Outdated Show resolved Hide resolved
api/migrations/schema/Version20240621195713.php Outdated Show resolved Hide resolved
api/src/Entity/Checklist.php Outdated Show resolved Hide resolved
api/src/Entity/ContentNode/ChecklistNode.php Show resolved Hide resolved
@pmattmann pmattmann force-pushed the feature/checklist branch 3 times, most recently from 59a361e to d080c5f Compare July 10, 2024 08:25
@pmattmann
Copy link
Member Author

Maybe if you had a transient field, api platform woud fetch the Items directly for you?

@BacLuc
Do you have any idea how to do this?

@BacLuc
Copy link
Contributor

BacLuc commented Jul 15, 2024

Maybe if you had a transient field, api platform woud fetch the Items directly for you?

@BacLuc Do you have any idea how to do this?

don't wait for this.
I just remembered that api platform uses the doctrine annotations to find out, what the related entitiy is.
Without that, it is difficult to find the relation. We can still change it later.

@pmattmann pmattmann requested a review from BacLuc July 15, 2024 17:04
@BacLuc BacLuc added api-performance-test! Run API Performance test deploy! Creates a feature branch deployment for this PR labels Jul 20, 2024
Copy link

github-actions bot commented Jul 20, 2024

Feature branch deployment currently inactive.

If the PR is still open, you can add the deploy! label to this PR to trigger a feature branch deployment.

Copy link
Contributor

@BacLuc BacLuc left a comment

Choose a reason for hiding this comment

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

Thank you very much

@pmattmann pmattmann added this pull request to the merge queue Aug 27, 2024
Merged via the queue into ecamp:devel with commit 48f912a Aug 27, 2024
33 of 34 checks passed
@pmattmann pmattmann deleted the feature/checklist branch August 27, 2024 17:56
@BacLuc BacLuc mentioned this pull request Sep 10, 2024
@manuelmeister manuelmeister mentioned this pull request Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-performance-test! Run API Performance test deploy! Creates a feature branch deployment for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants