Repository navigation
docs(mypolitics): rework the six tasks behind the open mypolitics-app PRs - #83
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
… PRs Sweep of the six pull requests still open in gi-org-pl/mypolitics-app. Rewritten, published as new issues, old pull requests and issues closed: - SurveyQuestion (gi-org-pl/mypolitics-app#87) - SurveyControls (gi-org-pl/mypolitics-app#88) - SurveyDemographics (gi-org-pl/mypolitics-app#89) Updated in place with a fix list, pull requests kept open: - Layout wrapper (gi-org-pl/mypolitics-app#10) - SurveyCategorySelect (gi-org-pl/mypolitics-app#37) - QuizCard (gi-org-pl/mypolitics-app#11) Each task now follows the current task shape, links the exact Figma frames and corrects the places where the first version contradicted the design or a reviewer's ruling. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c768d61 to
f728b54
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c768d61442
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export interface SurveyControlsProps { | ||
| title: string; // quiz title shown during CATEGORY_SELECT phase | ||
| phase: SurveyPhase; // current survey phase — drives center pill content | ||
| categoryName: string; // shown in pill during QUESTION_ANSWER (large screen only) | ||
| questionsLeftnCategory: number; // animated count shown in pill during QUESTION_ANSWER | ||
| answersCount: number; // used to disable back button when 0 | ||
| onPrevious: () => void; // called on back button click | ||
| onReset: () => void; // called when reset is confirmed in the modal | ||
| quizName: string; // shown in the pill when nothing else is, and named in the reset dialog | ||
| label?: string; // pill text for phases with no category, e.g. "Prawie koniec!" - passed translated | ||
| categoryName?: string; // current category - passed translated | ||
| questionsLeft?: number; // questions left in the category; optional | ||
| isPreviousDisabled?: boolean; // default false | ||
| isResetDisabled?: boolean; // default false |
There was a problem hiding this comment.
The existing cycle-2/survey-questionnaire.md still imports the removed SurveyPhase and Demographics types and instructs implementers to pass obsolete props such as phase, title, questionsLeftnCategory, questionDescription, isLoading, onSubmit, and onSkip (lines 15-16 and 110-122). Following it after these rewrites will not compile, and it still leaves demographic actions in the child even though this commit moves them to the parent; update that dependent task to provide the new labels/disabled flags, explanation prop, demographic options/values, and action row.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declined, with a reason. The facts are right: survey-questionnaire.md still names the old props. But that file is the myPolitics 1.0 task with four phases, and its issue (gi-org-pl/mypolitics-app#36) was closed as not planned on 6 October - the 2.0 questionnaire has seven phases and is to be cut from the phases model doc through a spec. Updating it here would mean writing that new task without the spec, so it is left untouched; nobody should build from it.
| - **A background image does not make a card always expanded.** The frame has an image card that collapses (with a chevron) and one that does not. Only `isAlwaysExpanded`, `isHighlighted` and a wide screen keep a card open. | ||
| - **The Lingui example imported from the wrong packages.** Macros come from `@lingui/react/macro` and `@lingui/core/macro`. Hand-built message objects are not a substitute. | ||
| - **`isMainAction` is removed.** The frame draws the play button filled on every card, so the prop has nothing left to switch. |
There was a problem hiding this comment.
Once this prop is removed, the dependent cycle-2/home-page.md task still requires isMainAction in the featured-card structure, mock object, and Definition of Done (lines 22, 97, and 245). Its prescribed MOCK_FEATURED_QUIZ: QuizCardProps will therefore fail TypeScript's excess-property check when that task is implemented, so remove the obsolete prop from the downstream task as part of this contract change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3a8876a. isMainAction is removed from the structure, the mock and the Definition of Done in home-page.md, and gi-org-pl/mypolitics-app#16 carries the same text.
| | Prompt, 1 | Wybierz {count} najważniejszy dla Ciebie temat. | Choose the {count} topic that matters most to you. | | ||
| | Prompt, 2-4 | Wybierz {count} najważniejsze dla Ciebie tematy. | Choose the {count} topics that matter most to you. | | ||
| | Prompt, 5 and more | Wybierz {count} najważniejszych dla Ciebie tematów. | Choose the {count} topics that matter most to you. | |
There was a problem hiding this comment.
For valid values above 20, these ranges prescribe incorrect Polish: maxSelection=22 needs the 2–4 form (najważniejsze ... tematy), while 12 needs the many form. Since every positive integer is accepted, describe the Lingui/CLDR one/few/many forms and add cases such as 12 and 22 rather than treating every value of 5 or more alike.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3a8876a. The copy table now names the plural categories (one, few, many) and says the form comes from Lingui's plural, not from ranges; the test case and the Definition of Done cover 1, 3, 5, 12 and 22. gi-org-pl/mypolitics-app#37 carries the same text.
| | `onCardClick` given | Clicking the card anywhere outside its buttons calls it | | ||
| | `onCardClick` given, keyboard | The logo or title is a real button that calls it, so nothing is mouse-only | |
There was a problem hiding this comment.
The interface permits onCardClick when neither title nor logoUrl is present, but this keyboard behavior only creates a control from the logo or title, leaving that valid card mouse-only. A logo without a title is also permitted and produces an unnamed button because the task derives its alternative text from title; require an accessible card label/title for clickable cards or define a separate named keyboard target for these cases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3a8876a. A card is now clickable only when it has a title: the title names the button (also when a logo is shown), and without one onCardClick has no effect for mouse or keyboard - so the card is never mouse-only and never an unnamed control. Table, test cases and Definition of Done updated; gi-org-pl/mypolitics-app#11 carries the same text.
- HomePage: drop isMainAction, which QuizCard no longer has - SurveyCategorySelect: describe the prompt by plural category, so 12 and 22 take the right Polish form - QuizCard: a card is clickable only with a title, so it is never mouse-only and never an unnamed control Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
What
A sweep of the six pull requests that were still open in
gi-org-pl/mypolitics-app, all from May and June. Each was read against its task, its reviews, the Figma frame and today'sAGENTS.md, and got one of two verdicts. The six task files here are the result; the issues already carry the same text.Rewritten - new task, new issue, old pull request and old issue closed with a comment:
tasks/epics/survey/cycle-1/survey-question.mdtasks/epics/survey/cycle-1/survey-controls.mdtasks/epics/survey/cycle-1/survey-demographics.mdAdjusted - task corrected and extended with a fix list, pull request kept open:
tasks/epics/layout/cycle-2/layout.mdtasks/epics/survey/cycle-2/survey-category-select.mdtasks/epics/home-page/cycle-1/quiz-card.mdWhy each verdict
ButtonSelectand a separate "Sprawdź wyjaśnienie" button; the frame has neither. After seven review rounds the code still had helpers in the component file, DOM-measured heights, an explanation that could not be collapsed and malformed.pofiles.SurveyAnswer, the mount animation should be CSS, and the catalogs were not updated.Calls made
These were decided here and are worth a look, since no spec exists for these components - they predate the docs -> spec -> task pipeline.
Selecthas no clear action.nie, and an opened explanation can be collapsed again, as in 1.0.isMainAction(every play button in the frame is filled) and ties the image height to open or collapsed. The second is my reading of two cards in the frame.yarn e2erunnable, becauseAGENTS.md§4.6 puts that on the first route-reachable change.Notes for the reviewer
mypolitics/spec/, that is a separate step with its own review.main.🤖 Generated with Claude Code