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

Frontend: use API to show list of polling stations on polling station select page #146

Conversation

cikzh
Copy link
Contributor

@cikzh cikzh commented Jul 15, 2024

This PR replaces hard coded polling stations with polling stations provided by the backend. It also adds a lookup when the user enters a polling station number.

@cikzh cikzh force-pushed the 107-frontend-use-api-to-show-list-of-polling-stations-on-polling-station-select-page branch from f998f54 to e44a7cb Compare July 15, 2024 08:00
@praseodym praseodym changed the title 107 frontend use api to show list of polling stations on polling station select page Frontend: use API to show list of polling stations on polling station select page Jul 15, 2024
@cikzh cikzh force-pushed the 107-frontend-use-api-to-show-list-of-polling-stations-on-polling-station-select-page branch from e44a7cb to 6eace63 Compare July 15, 2024 10:12
@cikzh cikzh force-pushed the 107-frontend-use-api-to-show-list-of-polling-stations-on-polling-station-select-page branch from 6eace63 to ccaf062 Compare July 15, 2024 14:36
@cikzh cikzh force-pushed the 107-frontend-use-api-to-show-list-of-polling-stations-on-polling-station-select-page branch from ccaf062 to ff4eead Compare July 17, 2024 10:14
jschuurk-kr

This comment was marked as resolved.

@cikzh
Copy link
Contributor Author

cikzh commented Jul 17, 2024

When I hover over "Bekijk de lijst met alle stembureaus", I have an I-cursor (the text selecting one) instead of a pointer cursor.

That's weird, that was fixed here: #143. I'll rebase main

@cikzh cikzh force-pushed the 107-frontend-use-api-to-show-list-of-polling-stations-on-polling-station-select-page branch from ff4eead to 3f849e0 Compare July 17, 2024 13:34
@cikzh cikzh force-pushed the 107-frontend-use-api-to-show-list-of-polling-stations-on-polling-station-select-page branch from 3f849e0 to 38f93ef Compare July 17, 2024 13:46
@praseodym
Copy link
Contributor

Two things I noticed in my brief look at the preview environment:

  • The search design doesn't match the design in Figma.
  • If I enter a number that is not found, I can still continue to the data entry page.

@cikzh
Copy link
Contributor Author

cikzh commented Jul 18, 2024

@praseodym

* The search design doesn't match the [design in Figma](https://www.figma.com/design/zZlFr8tYiRyp4I26sh6eqp/Kiesraad---Abacus-optelsoftware?node-id=137-5073&t=HbflVzrg2pppVyBy-4).

Added the styling

* If I enter a number that is not found, I can still continue to the data entry page.

This will be fixed in #108

@cikzh cikzh marked this pull request as ready for review July 18, 2024 13:39
@cikzh cikzh requested a review from jorisleker July 18, 2024 13:39
@cikzh
Copy link
Contributor Author

cikzh commented Jul 24, 2024

  • the 'eerste/tweede invoer' badge after the name of the polling station is missing

@jorisleker I've fixed the design issues and added a debounce on the user input. I purposely left the badge out, as @jschuurk-kr mentioned earlier in a review that we should leave out design placeholders, and the badge logic is not implemented yet.

@jschuurk-kr
Copy link
Contributor

  • the 'eerste/tweede invoer' badge after the name of the polling station is missing

@jorisleker I've fixed the design issues and added a debounce on the user input. I purposely left the badge out, as @jschuurk-kr mentioned earlier in a review that we should leave out design placeholders, and the badge logic is not implemented yet.

My reasoning about the placeholders: I was starting to find it hard to look at the page, check what's in scope of the PR, identify which parts of the page had been implemented and which parts are placeholders, and then decide what to review/test for the PR. That left me with less mental bandwidth for testing/reviewing than I liked.

So for pages we're actively working on, I'm in favor of either removing placeholders or having placeholders that are clearly placeholders. In this case that could be the badge saying "invoerstatus" instead of "1ste invoer".

@praseodym
Copy link
Contributor

The PR description still says:

The PR is still in draft, because it lacks some test coverage, but it is otherwise ready for review.

I think that's outdated and can be removed?

@cikzh
Copy link
Contributor Author

cikzh commented Jul 24, 2024

  • the 'eerste/tweede invoer' badge after the name of the polling station is missing

@jorisleker I've fixed the design issues and added a debounce on the user input. I purposely left the badge out, as @jschuurk-kr mentioned earlier in a review that we should leave out design placeholders, and the badge logic is not implemented yet.

My reasoning about the placeholders: I was starting to find it hard to look at the page, check what's in scope of the PR, identify which parts of the page had been implemented and which parts are placeholders, and then decide what to review/test for the PR. That left me with less mental bandwidth for testing/reviewing than I liked.

So for pages we're actively working on, I'm in favor of either removing placeholders or having placeholders that are clearly placeholders. In this case that could be the badge saying "invoerstatus" instead of "1ste invoer".

In this case the Badge is typed in an object, so I'd have to add a placeholder field, or create a seperate placeholder badge, so I'll just leave it out for now. The Badges are still present in the code as commented TODO's, so they'll be trivial to find and implement, once we get to that.

@praseodym
Copy link
Contributor

Even though the UI says "SHIFT + Enter", a regular Enter keypress also submits (navigates to the data entry form). The data entry form also doesn't handle this, maybe @lkleuver has already resolved it there as part of #133?

@cikzh cikzh force-pushed the 107-frontend-use-api-to-show-list-of-polling-stations-on-polling-station-select-page branch from 027abf6 to 93c5df9 Compare July 24, 2024 15:53
@cikzh
Copy link
Contributor Author

cikzh commented Jul 24, 2024

Even though the UI says "SHIFT + Enter", a regular Enter keypress also submits (navigates to the data entry form). The data entry form also doesn't handle this, maybe @lkleuver has already resolved it there as part of #133?

This is now fixed

@cikzh
Copy link
Contributor Author

cikzh commented Jul 24, 2024

I think all feedback is either implemented or delegated to other issues

@cikzh cikzh added this pull request to the merge queue Jul 25, 2024
Merged via the queue into main with commit 95a2dd5 Jul 25, 2024
4 checks passed
@cikzh cikzh deleted the 107-frontend-use-api-to-show-list-of-polling-stations-on-polling-station-select-page branch July 25, 2024 08:31
@jorisleker jorisleker mentioned this pull request Jul 29, 2024
20 tasks
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.

Frontend: use API to show list of polling stations on polling station select page
6 participants