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

Contact us new #565

Merged
merged 6 commits into from
Nov 18, 2022
Merged

Contact us new #565

merged 6 commits into from
Nov 18, 2022

Conversation

hannahouazzane
Copy link
Contributor

Create contact page that matched design:
#557

@vercel
Copy link

vercel bot commented Nov 17, 2022

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

Name Status Preview Updated
neontribe-www ✅ Ready (Inspect) Visit Preview Nov 18, 2022 at 9:48AM (UTC)

import EmailLink from '../components/EmailLink'
const ContactUs = () => (
<Layout>
<PageMeta title="Contact us" description="Enter in suitable description" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you need to enter in a suitable description here? 😄

<img src={Twitter} height={42} width={42}></img>

<Text color="#561dee" size="normal">
<h4>@neontribe</h4>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a heading? There is some good guidance on how and where to use headings in Mozilla docs - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements#usage_notes

</Text>
<VerticalSpacing size={3}></VerticalSpacing>
<Container justifyContent="flex-start">
<Text color="#561dee">
Copy link
Contributor

Choose a reason for hiding this comment

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

Because I am a nerd and love using all the html elements, here would be a nice place to use the <address> element, which is specifically for the purpose of indicating the address details of an person or organisation. Totally optional fun added extra, you can checkout the docs here - https://developer.mozilla.org/en-US/docs/Web/HTML/Element/address

Copy link
Contributor

@rosejbon rosejbon left a comment

Choose a reason for hiding this comment

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

You are 🪨-ing it!

(Please excuse awful emoji puns)

@hannahouazzane hannahouazzane merged commit a99868e into develop Nov 18, 2022
@hannahouazzane hannahouazzane deleted the contact-us-new branch November 18, 2022 10:14
Copy link
Contributor

@Nikomus Nikomus left a comment

Choose a reason for hiding this comment

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

Some content and style suggestions

<a href={link}>{children}</a>

<style jsx>{`
div {
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting this styling on the <div> rather than the <a> will mean there's an area inside the div that looks clickable, but isn't.

Comment on lines +53 to +59
connect: PropTypes.string,
link: PropTypes.string.isRequired,
children: PropTypes.node,
border_color: PropTypes.string,
background: PropTypes.string,
margin_left: PropTypes.string,
mobileSize: PropTypes.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are all of these in use?

</div>

<EmailLink
link="/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a mailto:hello?

Comment on lines +53 to +64
<Text size="normal" color="black" maxCharacter="49ch">
Follow us on Twitter
</Text>

<div className="social-media-container">
<div className="social-media-labels">
<img src={Twitter} height={42} width={42}></img>

<Text color="#561dee" size="normal">
@neontribe
</Text>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a link to Twitter here? "Follow us on Twitter" sounds like a call to action, but there isn't anything to interact with. We can also help out screen readers with an alt attribute on the image.

Comment on lines +156 to +157
margin-right: 10vh;
width: 15vh;
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 using vertical height to style horizontally can have some unexpected effects, especially on large monitors.

<Container justifyContent="flex-start">
<Text color="#561dee">
<address>
<p>21 Colegate </p>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the old address, the new one is Norwich Office, Unit B, Seymour House, 30-34 Muspole Street, Norwich, NR3 1DJ

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.

3 participants