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

Badge pages #4389

Draft
wants to merge 15 commits into
base: develop
Choose a base branch
from
Draft

Badge pages #4389

wants to merge 15 commits into from

Conversation

schradert
Copy link
Contributor

@schradert schradert commented May 28, 2024

Closes #4318

  • backend: paginated ListBadgeUsers for retrieving badge's user list
  • frontend: badge index page
  • frontend: badge user page
  • frontend: tests
  • frontend: storybook
  • frontend: UI/UX improvements

Backend checklist

  • Formatted my code by running autoflake --exclude src/proto -r -i --remove-all-unused-imports src && isort . && black . in app/backend
  • Added tests for any new code or added a regression test if fixing a bug
  • All tests pass
  • Run the backend locally and it works
  • Added migrations if there are any database changes, rebased onto develop if necessary for linear migration history

Web frontend checklist

  • Formatted my code with make format
  • There are no warnings from make lint
  • There are no console warnings when running the app
  • Added any new components to storybook
  • Added tests where relevant
  • All tests pass
  • Clicked around my changes running locally and it works
  • Checked Desktop, Mobile and Tablet screen sizes

this is the backend needed to support pages to view all the users with a
particular badge (i.e. who are all the founders, board members, strong
verified people, etc.)
@schradert schradert added 2.has feature (new) Implementation of a feature 1.topic backend This issue relates to the python backend labels May 28, 2024
@schradert schradert added this to the V1 milestone May 28, 2024
@schradert schradert requested a review from aapeliv May 28, 2024 04:26
@schradert schradert self-assigned this May 28, 2024
@schradert schradert linked an issue May 28, 2024 that may be closed by this pull request
@schradert
Copy link
Contributor Author

@aapeliv I'm thinking that we probably need pagination here for badges with many users (phone verified, strong verification, etc.), what do you think? I can also just not query the database for these badges in #4318 and just show the number of user accounts or something.

@aapeliv
Copy link
Member

aapeliv commented May 29, 2024

Yes I think that's right. If you are down, pagination would be great!

@schradert schradert marked this pull request as draft May 30, 2024 15:01
@schradert schradert changed the title ListBadgeUsers API Badge pages May 30, 2024
Some badges could soon have thousands of users
Currently the workflow for using the local backend with the local
frontend is to replace `.env.development` with `.env.localdev` which
modifies the git working tree before running `yarn start`. Naturally
this must be undone when you commit, which is generally not good
development practice and also confusing that this would be necessary if
a script that specifies the right environment config file could be used
instead.

There is an [unfortunate
shortcoming](vercel/next.js#25764) in
NextJS for environment management, which is that ["non-standard"
NODE_ENV](https://nextjs.org/docs/messages/non-standard-node-env)
variables are not supported due to possible side effects. Because of
this, and the fact that the NODE_ENV is determined from the NextJS
command that is run, you cannot actually tell `yarn dev` to use a
different environment file than `.env.development` (this must be used!).

Fortunately, there is an [environment variable load
order](https://nextjs.org/docs/pages/building-your-application/configuring/environment-variables)
which we take advantage of here to inject the `.env.localdev` variables
before the `.env.development` ones (both files are loaded, but
`localdev` ones come first).

It's a straightforward workaround, but many wish it didn't have to be
this way. I preferred this solution over installing some dependency that
isn't really necessary, especially because it follows a NextJS standard.
consolidated hooks for the badges feature
I thought having a clickable chip with a tooltip is a common scenario we
might want to make use of later, so I extracted this functionality
originally built out in the features/profile/view/Badges file into a
ToolChipLink component.

The Badge component is then just a wrapper around that where you pass in
a Badge resource. This uses a URL scheme for badges at /badges/{id}
These are better user-facing descriptions of the available badges
aapeliv
aapeliv previously approved these changes Jun 2, 2024
Copy link
Member

@aapeliv aapeliv left a comment

Choose a reason for hiding this comment

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

The backend looks great to me! I haven't been able to run the frontend locally (my working directory is a mess with the notify v2 stuff). But happy to merge once it passes all the tests, or we can look for someone from frontend to have a quick look as well!

@@ -0,0 +1,44 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how we should go about localizing these. I guess doing this is not a bad idea. Then if we come up with a better way, we can always copy these translations over!

Copy link

vercel bot commented Jul 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
couchers ❌ Failed (Inspect) Jul 24, 2024 4:14am

Designed to look the same as the index page
also added some better styling to the instructions text on the index page
@nabramow
Copy link
Contributor

nabramow commented Nov 2, 2024

@schradert are you still working on this or is it okay if someone else takes it over?

@schradert
Copy link
Contributor Author

Feel free to reassign anything assigned to me @nabramow

@nabramow
Copy link
Contributor

nabramow commented Nov 2, 2024

@aapeliv What if we finish this ticket just as the backend, since Tristan looks like he mostly finished it. Then @bakeiro we can make a separate ticket for the frontend part of this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.topic backend This issue relates to the python backend 1.topic frontend 2.has feature (new) Implementation of a feature
Projects
Status: Abandoned
Development

Successfully merging this pull request may close these issues.

Page to list users with a given badge
3 participants