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

Display entry #7

Merged
merged 15 commits into from
May 14, 2022
Merged

Display entry #7

merged 15 commits into from
May 14, 2022

Conversation

NadineGrosskreuz
Copy link
Owner

Created first entry with static data and added storybook, styled components and testing.
User Story #1

@vercel
Copy link

vercel bot commented May 12, 2022

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

Name Status Preview Updated
capstone-project ✅ Ready (Inspect) Visit Preview May 13, 2022 at 1:38PM (UTC)

Copy link

@PReichetanz PReichetanz left a comment

Choose a reason for hiding this comment

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

Look really good, just have minor issues to consider 🚀

Comment on lines +8 to +9
<Paragraph>{entries.category}</Paragraph>
<HeadlineTwo>{entries.name}</HeadlineTwo>

Choose a reason for hiding this comment

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

I am confused by the logical order: A headline should be above the text it belongs to, shouldn't it?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I was also not sure if I could do it that way. I have taken the headline, because it is like the headline of the card, but the category as a p-tag should be above it. Would you recommend to use a p-tag instead of the headline?
The sketch from my user story perhaps shows better how i mean it:
Single entry

Choose a reason for hiding this comment

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

I agree that you use a headline, I was just wondering about the order. I would still suggest to put a headline above everything, but this is rather a design question. Maybe you can leave it this way and check for accessibility before merging.

Copy link
Owner Author

Choose a reason for hiding this comment

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

What do you mean by check for accessibility before merging? Checking with a screen reader?
It would also make sense to read Name (Headline), Kategorie and Adresse.
But yes, I did this for design reasons.

import Card from './Card';

describe('Card', () => {
it('renders category,name and address', () => {

Choose a reason for hiding this comment

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

Suggested change
it('renders category,name and address', () => {
it('renders category, name, and address', () => {

);

const category = screen.getByText(/Flohmarkt/i);
const name = screen.getByText(/Faust/i);

Choose a reason for hiding this comment

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

Since you use a h2 element for this, you could try getByRole('heading', {name: /Faust/i}); this query is the number 1 priority have a look here.

}

html {
font-size: 62.5%;

Choose a reason for hiding this comment

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

Why would you scale down the font-size globally? Is your app still accessible (i.e. readable for people you ave problems with small fonts)?

Copy link
Owner Author

@NadineGrosskreuz NadineGrosskreuz May 13, 2022

Choose a reason for hiding this comment

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

Our head couch told us to do so, so that it is easier to work with rem.
A fellow student sent me this article about it: https://www.aleksandrhovhannisyan.com/blog/62-5-percent-font-size-trick/
But since I want to pay special attention to accessibility, I am open to changing that if it does affect accessibility.

Choose a reason for hiding this comment

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

Ok, I didn't know about this, so I think it's fine :)

@NadineGrosskreuz NadineGrosskreuz merged commit 102efa1 into main May 14, 2022
@NadineGrosskreuz NadineGrosskreuz deleted the display-entry branch May 14, 2022 13:42
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.

None yet

3 participants