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/update design #11

Closed
wants to merge 6 commits into from
Closed

Feat/update design #11

wants to merge 6 commits into from

Conversation

mwogates
Copy link
Contributor

No description provided.

@mwogates mwogates requested a review from roienatan May 16, 2022 20:19
@mwogates mwogates self-assigned this May 16, 2022
@mwogates mwogates linked an issue May 16, 2022 that may be closed by this pull request
.contactus-form {
width: 100%;
max-width: 386px;
&__title {
Copy link
Contributor

@roienatan roienatan May 17, 2022

Choose a reason for hiding this comment

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

I would strongly recommend to use full snake case for example instead of &__title so contactus-form__title. This way it's super easy to find the right class in case of debugging/making changes. Besides I think it's very confusing that half of the project is written in certain way and the other in different way - so in my opinion we have to go with one way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

export default function ContactUs() {
const { t } = useTranslation();

return (
Copy link
Contributor

Choose a reason for hiding this comment

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

No send button to this form?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 17, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6c361f6
Status: ✅  Deploy successful!
Preview URL: https://c0dc314c.web-2l1.pages.dev

View logs

<label>{t("Landing.ContactUs.position")}</label>
<input type="text" />
</div>
<button className="button contactus-form__button">
Copy link
Contributor

Choose a reason for hiding this comment

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

@mwogates Let's just make the button disabled when at least one of the fields is empty.
I would also add validation for the email address with a red error in case it's invalid email. The send button should be disabled in that case too.

@mwogates mwogates requested a review from roienatan May 17, 2022 20:57
@roienatan
Copy link
Contributor

@mwogates I've pushed some small updates. Anyhow as we still discuss about the way the newsletter will work so this PR will be on hold.

@roienatan roienatan marked this pull request as draft July 31, 2022 12:16
@shayzluf shayzluf closed this Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Website design corrections
3 participants