Allow adding links to logos #278
Conversation
Link Check ReportThere were no links to check! |
| src: fastapiLogo, | ||
| alt: 'FastAPI logo' | ||
| alt: 'FastAPI logo', | ||
| link: '/doc/user-guide/serving/fastapi' |
There was a problem hiding this comment.
Thanks a lot @yathomasi! I see the border is around logos, can we make them around boxes for logos maybe? Otherwise it looks inconsistent somehow
and
There was a problem hiding this comment.
Yes, great suggestion. I have updated it a bit. Let me how you think.
There was a problem hiding this comment.
Thanks! Looks much better! Could we also make them always the same height? Now this is how it looks for me:
and
There was a problem hiding this comment.
Thanks a ton! Works as expected now! Now I need to get feedback from @iterative/mlem and update "Working with models" page.
src/components/Home/Header/index.tsx
Outdated
| @@ -114,94 +124,130 @@ const logosData: Array<{ | |||
| widthMd: 88, | |||
| widthLg: 124, | |||
| src: rabbitmqLogo, | |||
| alt: 'RabbitMQ logo' | |||
| alt: 'RabbitMQ logo', | |||
| link: '/doc/user-guide/serving/rabbitmq' | |||
There was a problem hiding this comment.
This page is empty. Maybe not add a link at all? https://mlem.ai/doc/user-guide/serving/rabbitmq
There was a problem hiding this comment.
I would add a link once there is a bit more content (like linking to a working example and some explanations of how it works and how to build an app with it)
src/components/Home/Header/index.tsx
Outdated
| }, | ||
| { | ||
| widthSm: 66, | ||
| widthMd: 88, | ||
| widthLg: 124, | ||
| src: onnxLogo, | ||
| alt: 'ONNX logo' | ||
| alt: 'ONNX logo', | ||
| link: '/doc/user-guide/models' |
There was a problem hiding this comment.
I think we need to add mentions of all ML frameworks to https://mlem.ai/doc/user-guide/models
So these links are meaningful.
There was a problem hiding this comment.
Agree, the existing model object-references pages e.g. https://mlem.ai/doc/object-reference/model/onnx are pretty useless as far as user docs go and not worth linking to from the front page
There was a problem hiding this comment.
Looks good to me. I assume we need a review from @rogermparent as well?
| {logosData.map((logoDetails, i) => | ||
| logoDetails?.link ? ( | ||
| <Link | ||
| href={logoDetails.link} | ||
| key={i} | ||
| className={cn( | ||
| styles.header__list, | ||
| 'hover:border-purple-800 active:bg-gray-200 transition-colors' | ||
| )} | ||
| > | ||
| <LogoImage {...logoDetails} /> | ||
| </Link> | ||
| ) : ( | ||
| <li key={i} className={cn(styles.header__list)}> | ||
| <LogoImage {...logoDetails} /> | ||
| </li> | ||
| ) | ||
| )} |
There was a problem hiding this comment.
Theoretically if any logoDetails were undefined, this would blow up with a weird error as it attempts to spread an undefined logoDetails. That said, that specific scenario is probably not going to happen.
cc: @aguschin