Fix: open participation page accessibility issues (Axe) #32

thomassth · 2024-10-17T06:43:16Z

Fix https://github.com/orgs/CivicTechTO/projects/4/views/1?filterQuery=parti&pane=issue&itemId=82264013

WAVE highlighted more items & would need more structural change, will do in another commit.

…ailing Docker build in some environments. (#33)

patcon · 2024-10-17T19:08:07Z

@thomassth i think may have intended to base this PR off of what is now called edge, instead of the current edge-civictechto (the prior edge branch accidentally had some extra commits from me, so I renamed it to be more clearly our branch, and locked the new edge to only sync with upstream. Hopefully this avoids future confusion

I don't think compdem will accept your PR upstream until you rebase, as there's some unrelated commits related to our storybook prep in there :)

NewJerseyStyle · 2024-10-17T19:35:24Z

About branch name, sorry I didn't state clearly in the documentation before.

When you create a branch:

If you are fixing a bug

Name the branch with fix/issue-<issue number>-<key word of issue> (e.g. fix/issues-25-display-button)

If you are adding feature described in issues

Name the branch with feature/<key word about feature>-issue-<issue number> (e.g. feature/a11y-module-issue-13)

If the branch does not associate to any issue

We will get confused

Creating PR

Create pull request to our edge branch from your repository/branch with changelog (so we know what you did, then we can verify your changes and test the code for integration test) in case changelog is too trivial for you, leverage AIs to write it is also a good idea. 🙇

More about development

Fixes issue with docker build failing due to makefile typo

NewJerseyStyle · 2024-10-17T20:17:49Z

Assigned @patcon as reviewer, assuming @thomassth wants to help in patcon's branch

(cherry picked from commit 4eb7c20)

(cherry picked from commit 1d47c7a)

(cherry picked from commit a0fcfb4)

(cherry picked from commit e1e5112)

thomassth · 2024-10-18T05:00:12Z

I'm still confused which branch are we working on right now

NewJerseyStyle · 2024-10-18T05:03:16Z

Now I guess the edge branch we used to use has been renamed as edge-civictechto but all description in documents were not updated so everyone was confused.

patcon · 2024-10-18T08:35:19Z

Ah I'm sorry! that was my fault for changing it without checking for docs to update 😞

As I was doing it, I was wishing we had a PR-based way to edit settings, so the setings change could be discussed like a clear code change. I've used this on other projects: https://github.com/repository-settings/app (repo config-as-code)

But respectfully, I think this situation (accidentally pushing extra commits upstream) is kinda unrelated to what I did, no? we can't submit PR feature branches to upstream that have been branched off our active (and different) edge branch (whether we call it edge or edge-civictechto or whatever). Yes, I tried to clean up our commits in edge, but i believe someone immediately pushed the old commits back onto it? and avoiding that situation is at the core of what we need to resolve about this in our workflow, i think? (If I'm misunderstanding, I apologize!)

Regardless, sorry for generating an extra layer of confusion around something that was already confusing...! Appreciate y'all!

patcon · 2024-10-18T08:44:31Z

For now:

I merged the remote tracking branch remote/civictechto/edge into our edge-civictechto
I merged edge-civictechto into this feature branch

So now this PR is clean again 🎉

client-participation/js/templates/link-TOS-partial.handlebars

patcon · 2024-10-18T08:46:38Z

client-participation/js/templates/link-TOS-partial.handlebars

  {{s.TOS}}
-</a>
+</a>


nit: pls revert (will explain later)

client-participation/js/templates/link-privacy-partial.handlebars

patcon · 2024-10-18T08:47:21Z

client-participation/js/templates/link-privacy-partial.handlebars

+    "
+>{{s.privacy}}</a>


nit: pls revert (will explain later)

patcon · 2024-10-18T08:49:06Z

client-participation/vis2/components/curate.js

@@ -8,7 +8,7 @@ class Button extends React.Component {
    this.props.handleCurateButtonClick(this.props.identifier)
  }

-  render () {
+  render() {


nit: pls revert (will explain later)

patcon · 2024-10-18T08:49:16Z

client-participation/vis2/components/curate.js

        color: (!_.isNull(this.props.selectedTidCuration) && this.props.selectedTidCuration === this.props.identifier) ? "rgb(255,255,255)" : "rgb(100,100,100)",
        borderRadius: 4,
      }}
-      onClick={this.handleClick.bind(this)}>
+        onClick={this.handleClick.bind(this)}>


nit: pls revert (will explain later)

patcon · 2024-10-18T17:23:49Z

No no it's my fault. I was absent this week (had a bad mental health week, and needed to step back). I'll process your message and respond in the issue specifically about branching to keep things on-topic here :)

NewJerseyStyle · 2024-10-18T17:39:45Z

We can see, and I am sure people are happy to offer assist for mental issues (hiking, walking, talking etc.) if we know how @patcon

On the other hand, let me organize the events related to branches

One day patcon proposed that we should have a branch to keep track upstream, edge was proposed to do the job
And I proposed that we should use dev branch to merge all PRs before merge to main, dev was proposed to do the job
Some members told me edge should be where we keep the latest development
We (patcon wasn't there) agreed to use edge to track the latest development of all sources (from compdemocracy/polis and from PRs of our member) to keep things easy to manage
dev is merged into edge
edge is merged into main
all PRs merge to edge
edge is renamed to edge-civictechto
We are confused
Here, now, this thread. It seems we had different expectation about what edge should be. So can you review and verify my understanding mentioned in above comment? 🙏 thanks

patcon · 2024-10-18T20:19:18Z

For others: I responded back in #35

patcon · 2024-10-18T20:21:06Z

FYI, when I tried to merge yesterday to made this PR cleaner, the upstream PR got messier: compdemocracy#1821 (I don't think we can do PRs to both with one branch in our current flow)

NewJerseyStyle · 2024-10-18T20:58:41Z

I am not sure what do you mean by messier, to me the issue would be github action script, everything else doesn't kill anyone so they can be resolved easily somehow

NewJerseyStyle · 2024-10-18T21:07:41Z

After fixing branching issues, E2E test for the merge of this PR reported failure. I created another issue for it: #39

Zen-cronic · 2024-10-18T21:57:54Z

heh ok, i didn't go straight to the end, but the ideal contributor etiquette is to not let your editor make a bunch of noisy changes that are not actually what you are asking to get in, and make review as clean and small and simple as it can be for maintainers :)

...
If you have a codestyle change or cleanup you want to get into the codebase, I'd suggest doing that in one go, and having a PR and separate review specifically for all those fixups. Fixing them up on the fly in unrelated PRs is often not considered as helpful as it might appear, esp on a project like polis where PRs need to be really clean and in-scope

Agree, we appreciate your effort and contribution @thomassth, but clean PRs get reviewed and merged relatively faster in FOSS! also it's easier to spot actual code changes and bugs that way.

thomassth · 2024-10-18T23:17:06Z

I was gonna rebase & clean it up the linting after work, but it got merged already!

Since this is merged, I'll reuse my branch to fix up the upstream PR (just realized I didn't open it in my own account 🙈)

imo we should have a linting default, but it's not a must. And we do that, we probably should do a cleanup in a separate styling PR before enforcing such things.

NewJerseyStyle · 2024-10-18T23:21:30Z

I see. Sorry to merge it too soon after fixing branching problem.

And the Lint and other checks defined in GitHub Action was not triggered on the new branch name, I am fixing them

client-participation/js/templates/participation.handlebars

Fixed issue with Makefile prefixing extra whitespace to envvars and f…

e243833

…ailing Docker build in some environments. (#33)

Merge pull request #34 from patcon/33-makefile-fix

d350855

Fixes issue with docker build failing due to makefile typo

NewJerseyStyle changed the base branch from edge-civictechto to edge October 17, 2024 20:14

NewJerseyStyle changed the base branch from edge to edge-civictechto October 17, 2024 20:15

NewJerseyStyle requested a review from patcon October 17, 2024 20:16

thomassth added 5 commits October 18, 2024 00:44

increase text contrast

6498485

increase footer text contrast

40a6349

(cherry picked from commit 4eb7c20)

HTML landmarks, h1 heading

7893aee

(cherry picked from commit 1d47c7a)

add title to document

81988d1

(cherry picked from commit a0fcfb4)

increase active primary button contrast

ab1330a

(cherry picked from commit e1e5112)

thomassth force-pushed the participation-a11y branch from e1e5112 to ab1330a Compare October 18, 2024 04:48

increase submit button contrast

bd9c197

thomassth changed the title ~~Participation a11y~~ Fix: open participation page accessibility issues Oct 18, 2024

thomassth changed the title ~~Fix: open participation page accessibility issues~~ Fix: open participation page accessibility issues (Axe) Oct 18, 2024

remove unnecessary form role

c5cfbc4

Merge branch 'edge-civictechto' into participation-a11y

6f7a5af