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: tolocars #83

Open
wants to merge 48 commits into
base: main
Choose a base branch
from
Open

feat: tolocars #83

wants to merge 48 commits into from

Conversation

Agustina-Carrion
Copy link
Member

added line clamp and checked for some styling of the svg operator icons. I still haven't started with the single view/dynamically fetched logic addition. Just for having a first glimpse of the tolocars index.

Copy link
Member

@pReya pReya left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-02-03 at 11 59 17

The width of the grid area should be just as wide as the content-width/header on large desktop viewports.

Copy link
Member

@pReya pReya left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-02-03 at 11 58 47

The tags need to be as wide as their text content (but obviously the tag should never break onto two lines).

Copy link
Member

@pReya pReya left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-02-03 at 11 58 38

Getting an empty tag here. Not sure why. Maybe the word is just too long?

@Agustina-Carrion
Copy link
Member Author

Screenshot 2023-02-03 at 11 58 38

Getting an empty tag here. Not sure why. Maybe the word is just too long?
Nope, there's a bug i need to figure out 😅.. this card should only get one tag... the Boolean(array.length) is not working. I might be skipping a step to actually make it work 🤔

@pReya
Copy link
Member

pReya commented Feb 3, 2023

Screenshot 2023-02-03 at 11 58 38 Getting an empty tag here. Not sure why. Maybe the word is just too long? Nope, there's a bug i need to figure out 😅.. this card should only get one tag... the Boolean(array.length) is not working. I might be skipping a step to actually make it work 🤔

This is the reason:

            tags={[
              tolocar.frontmatter?.tags?.[0],
              tolocar.frontmatter?.tags?.[1],
            ]}

This will always create an array of length 2. If the input values don't exist it will be ["bla", undefined] – which is still an array of length 2.

So, instead of this, we should do: tags={tolocar.frontmatter?.tags?.slice(0,2)}

Also inside of your card make sure that undefined, null or "" are not rendered at all.

@Agustina-Carrion
Copy link
Member Author

Screenshot 2023-02-03 at 11 58 38 Getting an empty tag here. Not sure why. Maybe the word is just too long? Nope, there's a bug i need to figure out 😅.. this card should only get one tag... the Boolean(array.length) is not working. I might be skipping a step to actually make it work 🤔

This is the reason:

            tags={[
              tolocar.frontmatter?.tags?.[0],
              tolocar.frontmatter?.tags?.[1],
            ]}

This will always create an array of length 2. If the input values don't exist it will be ["bla", undefined] – which is still an array of length 2.

So, instead of this, we should do: tags={tolocar.frontmatter?.tags?.slice(0,2)}

Also inside of your card make sure that undefined, null or "" are not rendered at all.

I reckoned that was the issue! Just didn't know how to address it. 🤔 And since it's also in the interventions PR, we might need to change it to prevent bugs in the case of more than 2 tags.

Copy link
Member

@pReya pReya left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-02-03 at 14 14 56

Can you move the truck image a little bit more to the top? Also the paragraph in this box is supposed to be in the aktiv font.

date,
tagIcon,
operatorIcon,
Copy link
Member

Choose a reason for hiding this comment

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

Can we just name this operators? They're not really icons, but logos in my opinion. Also what happens if we have multiple operators (there's one of these examples in the Figma design)? I'd make this an array.

className || ""
}`}
>
{tagIcon && (title === tagIcon ? renderIcon(tagIcon) : null)}
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this. This means that the icon will only be rendered when the title and the tagIcon are the same. What's the reasoning here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm struggling with the tagIcon... the idea to attach the icon to the corresponding tag, based on a series of words representing each tagIcon. I didn't try to make it work yet, I left it there to revisit. I might bring it up when we chat briefly.

title: string;
subtitle?: string;
date?: string;
footerGrey?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

In the Layout files, you're hardcoding footerGrey to be true. So we can't change it via frontmatter anymore. I would just remove it from this type.

base.css Outdated
Comment on lines 126 to 129
.background-fleet {
background-image: url("/truck.svg");
background-size: 94.87px 75.38px;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be done inline as well with something like bg-[size:94.87px_75.38px] bg-[url('truck.svg')]

@tobimori
Copy link
Member

tobimori commented Feb 4, 2023

Screenshot 2023-02-03 at 14 14 56

Can you move the truck image a little bit more to the top? Also the paragraph in this box is supposed to be in the aktiv font.

Not sure why you said the paragraph should be in the Aktiv font. It's Inter in the design.

Edit: My Figma was buggy and displaying it as Inter. I updated the design accordingly.
Make sure there's secondary text (not bold) with white & 70% opacity, while bold text is 100% opacity.
We don't use our color scale here to match with the background.

CleanShot 2023-02-04 at 19 34 16@2x

Design for reference

@pReya pReya temporarily deployed to ionos February 10, 2023 15:12 — with GitHub Actions Inactive
@pReya pReya temporarily deployed to ionos February 10, 2023 15:13 — with GitHub Actions Inactive
@github-actions
Copy link

✅ Successfully deployed to: https://dev.tolocar.org/feat-tolocars

@pReya pReya changed the title feat: tolocars subpage feat: tolocars Feb 10, 2023
@tobimori
Copy link
Member

Please check the behaviour of the cards here:
CleanShot 2023-02-11 at 09 52 59@2x

The image is supposed to be the same height on all trucks, but when content is longer, blocks in the same row that are smaller should expand and fill the height.

Design for reference
CleanShot 2023-02-11 at 09 53 47@2x


CleanShot 2023-02-11 at 09 54 39@2x

These labels also have the wrong letter spacing (it's +0,08em for better reading) and wrong font weight (600 in the design). Please update accordingly.

Responsive behavior of the banner here is still broken (no margin bottom)
CleanShot 2023-02-11 at 09 55 55@2x
Description is meant to be smaller and Inter on mobile

CleanShot 2023-02-11 at 09 56 30@2x

On Tablet, please have smaller margin bottom for the banner (64px or 80px)

@Agustina-Carrion Agustina-Carrion temporarily deployed to ionos February 14, 2023 16:17 — with GitHub Actions Inactive
@Agustina-Carrion Agustina-Carrion temporarily deployed to ionos February 14, 2023 16:19 — with GitHub Actions Inactive
@github-actions
Copy link

✅ Successfully deployed to: https://dev.tolocar.org/feat-tolocars

#TC2 no license plate
@Anna2608 Anna2608 temporarily deployed to ionos May 15, 2023 14:51 — with GitHub Actions Inactive
@Anna2608 Anna2608 temporarily deployed to ionos May 15, 2023 14:51 — with GitHub Actions Inactive
@pReya pReya temporarily deployed to ionos May 30, 2023 08:50 — with GitHub Actions Inactive
@pReya pReya temporarily deployed to ionos May 30, 2023 08:51 — with GitHub Actions Inactive
@github-actions
Copy link

✅ Successfully deployed to: https://dev.tolocar.org/feat-tolocars

@pReya pReya temporarily deployed to ionos May 31, 2023 14:27 — with GitHub Actions Inactive
@pReya pReya temporarily deployed to ionos May 31, 2023 14:27 — with GitHub Actions Inactive
@github-actions
Copy link

✅ Successfully deployed to: https://dev.tolocar.org/feat-tolocars

@Anna2608 Anna2608 temporarily deployed to ionos June 2, 2023 09:20 — with GitHub Actions Inactive
@pReya pReya temporarily deployed to ionos June 3, 2023 07:37 — with GitHub Actions Inactive
@pReya pReya temporarily deployed to ionos June 3, 2023 08:20 — with GitHub Actions Inactive
@pReya pReya temporarily deployed to ionos June 3, 2023 08:22 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Jun 3, 2023

✅ Successfully deployed to: https://dev.tolocar.org/feat-tolocars

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