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

Challenge Page #158

Merged
merged 40 commits into from
May 4, 2024
Merged

Challenge Page #158

merged 40 commits into from
May 4, 2024

Conversation

lucapircher
Copy link
Member

@lucapircher lucapircher commented Oct 6, 2023

Description

  • adds all the supporting infrastructure for using the app router and the pages router
    • graphql authentication
    • authentication
    • application configuration
    • adjusts the navigation, breadcrumbs, etc. so that they are compatible with the pages and the app raut
  • creates a dedicated page for the challenges using the nextjs app router

Closes https://github.com/a11yphant/issues/issues/46

Review App: https://a11yphant-158-merge.cluster.a11yphant.com

Definition of Done

General

  • Code Review
  • Test Coverage is equal or higher
  • Update ticket on the TODO board
  • (if .env file changes) Notify other team members

Site

  • Works in Firefox, Chrome and Safari
  • No W3C Errors (Wave Check)
  • Focus, Hover & Active Styles are present
  • Correct Tab Order
  • All relevant elements are focusable
  • Screen reader only reads relevant infos (no SVGs, ...)

@lucapircher lucapircher force-pushed the feature/challenge-page branch 2 times, most recently from 9f54531 to 62ea89f Compare October 7, 2023 14:27
services/site/src/middleware.ts Outdated Show resolved Hide resolved
services/site/src/middleware.ts Outdated Show resolved Hide resolved
@dnikub dnikub added the Site Part of the application: Site label Oct 7, 2023
@lucapircher lucapircher mentioned this pull request Oct 21, 2023
10 tasks
@dnikub
Copy link
Member

dnikub commented Oct 23, 2023

I would add 2 things for improved usability:

  1. When a level within the challenge is completed and gets the checkmark icon, I'd add SR text that says this level is already completed. currently, it only says that the link was visited, but that means nothing in this context.
  2. The one level that is coloured purple: I'd add the text "up next"/"start here" when it is the first one, or smth similar. otherwise, it is not completely clear why this one level is colored. Especially when no level was completed yet, is it not initially clear that one can click on the level to start (and we removed the start button).

Otherwise, it looks very good. We should, however, add a new ticket to add a skip link to the newest not completed level so you don't have to tab so much with your keyboard.

@lucapircher
Copy link
Member Author

Thanks for reviewing.

I would add 2 things for improved usability:

  1. When a level within the challenge is completed and gets the checkmark icon, I'd add SR text that says this level is already completed. currently, it only says that the link was visited, but that means nothing in this context.

Ah yeah that makes a lot of sense, I'll add that.

  1. The one level that is coloured purple: I'd add the text "up next"/"start here" when it is the first one, or smth similar. otherwise, it is not completely clear why this one level is colored. Especially when no level was completed yet, is it not initially clear that one can click on the level to start (and we removed the start button).

I'll also add that. Should we add that to the main text in the card? I added it in the devtools in screenshot below and I kinda like it.

image

Otherwise, it looks very good. We should, however, add a new ticket to add a skip link to the newest not completed level so you don't have to tab so much with your keyboard.

Ah sorry, I started replying to the other comment before I saw this. Yeah doing that in a dedicated ticket is fine by me and it would be a great addition.

@lucapircher
Copy link
Member Author

@thomasdax98 It would be great if you could also do a review for this one, since it will act as the foundation for future pages.

Copy link

codeclimate bot commented May 4, 2024

Code Climate has analyzed commit 95d22d5 and detected 12 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Duplication 11

The test coverage on the diff in this pull request is 89.8% (50% is the threshold).

This pull request will bring the total coverage in the repository to 90.8% (0.0% change).

View more on Code Climate.

@lucapircher lucapircher merged commit 6172bdc into develop May 4, 2024
20 checks passed
@lucapircher lucapircher deleted the feature/challenge-page branch May 4, 2024 09:34
@dnikub dnikub mentioned this pull request May 4, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Site Part of the application: Site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants