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

feat: add link for typeform datenschutz and impressum #25

Merged
merged 5 commits into from
Feb 15, 2024

Conversation

aeschi
Copy link
Contributor

@aeschi aeschi commented Feb 8, 2024

  • typeform link only shows when german is selected

- typeform link only shows when german is selected
@aeschi aeschi requested a review from raphael-arce as a code owner February 8, 2024 14:11
Copy link

vercel bot commented Feb 8, 2024

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

Name Status Preview Comments Updated (UTC)
buergeramt-goes-digital ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 15, 2024 0:30am

Copy link
Contributor

@raphael-arce raphael-arce left a comment

Choose a reason for hiding this comment

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

Got a couple of suggestions 👍 let me know what you think :)

src/App.tsx Outdated
@@ -31,6 +34,20 @@ function App() {
<Steps />
</main>

{language === availableLanguages[0] && (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rather use a named variable here, e.g. availableLanguages['de-DE], to make it easier to understand which language is used, and also to make sure no type error occurs. I looked a bit into it and realized the current state of implementation won't allow it. I think we'd need to use zod native enums instead of zod enums to type the availableLanguages. This is probably a bigger change than what is scoped in this ticket so maybe we can discuss and prioritize it first before implementing it.

src/App.tsx Outdated
Beantworte uns gerne einige Fragen.
</a>
</div>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make sense to move what is above in its own component, e. g. <Feedback />👍

src/App.tsx Outdated
rel="noopener noreferrer"
>
Impressum
</a>
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Maybe let's move what is above in its own component (e.g. <Footer />) to keep the App.tsx short and easy to understand.

- removed footer and feedback from App.tsx to individual components
- added all text sections to translation.json for clean structure
@raphael-arce
Copy link
Contributor

thanks 👍 lgtm!

<div className="flex w-full flex-wrap justify-center gap-x-2 px-8 py-4 text-sm">
{t("feedback.question", language)}
<a
className="text-blue-500 underline"
Copy link
Contributor

Choose a reason for hiding this comment

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

@aeschi just noticed that this is not the same color than other links, the other links are text-blue-700 to have enough contrast

Copy link
Contributor

Choose a reason for hiding this comment

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

Woops, my bad, I only I compared it with the download links on the checklist overview page. The link colors are inconsistent, sometimes 500, sometimes 700. Should we stick to one? Or does it make sense to go for a darker tone when the backgground is darker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's try to stick to one color with the most contrast and check again for the darker backgrounds. But technically a darker background would be better with a brighter text color.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I agree 👍

Copy link
Contributor

@raphael-arce raphael-arce left a comment

Choose a reason for hiding this comment

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

thx, lgtm 👍

@aeschi aeschi merged commit eb1e8af into main Feb 15, 2024
4 checks passed
@raphael-arce raphael-arce deleted the feat/add-typeform-link branch February 20, 2024 16:32
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.

2 participants