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

Survey Page #1636

Closed
wants to merge 63 commits into from
Closed

Survey Page #1636

wants to merge 63 commits into from

Conversation

henrycatalinismith
Copy link
Collaborator

@henrycatalinismith
Copy link
Collaborator Author

henrycatalinismith commented Nov 24, 2023

I dug myself quite a deep grave here last week by making so many different PRs against this branch at once. Each time I merged any of them the others got new merge conflicts to resolve! Down to the last few now. Happy to keep adding more stuff to the To Do list for this so don't worry about burning me out. Keen to really push and give this a whole bunch of my energy.

@henrycatalinismith
Copy link
Collaborator Author

henrycatalinismith commented Dec 10, 2023

Right then, we're in very good shape now. Let's take a quick inventory of what I'm aware of in terms of the main outstanding issues for this feature.

  • It contains two parallel radio button style implementations. One is in <SurveyOptionsQuestion /> and the other is in <SurveySignature />. While we do need those to be separate components it might be nice if they could use a shared component for their actual radio button UI rendering work.
  • Lots of the typography details are being handled as custom style={} props which are duplicated across multiple components. These should probably be refactored to something tidier.
  • Visually it's a long way from parity with what's in Figma. We talked briefly about this and I understand that the gold standard for how this should look is something of a moving target right now. Personally I'm happy to not touch this detail for the time being and would also be excited about letting it be a nice warm-up task for new contributors.
  • It needs to be possible to access the survey without being logged in, but that's not currently possible. We spoke about migrating src/pages/o/[orgId]/surveys/[surveyId]/index.tsx to the Next.js app router as part of addressing this. I've forgotten some of the details of that conversation since but I've poked around with it in a branch for a few minutes.
  • You mentioned that the PR hasn't used the normal translation workflow. Could be nice to sort that out but I'm actually not sure of the next steps there.

Really stoked about the progress we've made on this, and feeling very optimistic and open-minded about how to deal with the rest of it.

@henrycatalinismith
Copy link
Collaborator Author

Closing this now as today's hackathon progress enables us to consolidate these efforts into #1756 rather than continuing with multiple parallel threads of development.

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.

5 participants