feat: Link bus tooltip to line profile #463
Conversation
| @@ -39,7 +47,7 @@ export function BusToolTip({ position, icon }: BusToolTipProps) { | |||
| <> | |||
| <header className="header"> | |||
| <h1 className="title"> | |||
| {t('line')} :<span>{siriRide && siriRide.gtfsRouteRouteShortName}</span> | |||
| {t('line')} :<span>{LinkToProfile}</span> | |||
There was a problem hiding this comment.
| {t('line')} :<span>{LinkToProfile}</span> | |
| {t('line')} :<span> | |
| <Link to={`/profile/${siriRide.gtfsRouteRouteShortName}`}> | |
| {siriRide && siriRide.gtfsRouteRouteShortName}{' '} | |
| </Link> | |
| </span> |
isn't it easier?
There was a problem hiding this comment.
What if the siriRide is undefined?
I think that at some point of the renders it doesn't got any properties or even a value.
I can take care of the null/undefined scenario when using the Link component in an inline JSX, but I personally just like it less & have no problem to follow the project standard style.
There was a problem hiding this comment.
What about turning it into a component?
There was a problem hiding this comment.
I think that This is a good choice. I'll add it as a local Component.
There was a problem hiding this comment.
Actually, if the siriRide is undefined, we should probably hide the entire page. Don't you think?
There was a problem hiding this comment.
Yes there is no reason to show the tooltip without data. In the case I'll return NULL then there will be a white line which is the leaflet popup wrapper. So in a future issue it might be a nice to deal with it. For now it should be a good solution.
There was a problem hiding this comment.
I think it would be for the best if you'll place the link inline, and the nullish logic will be handled in outer scope
|
(accepted with suggestion - that's a great PR, thanks for the contribution, feel free to merge it with or without the suggestion. you're great, thank you) |
Added additional check suggestion Co-authored-by: Noam Gaash <[email protected]>
| {t('line')} :<span>{siriRide && siriRide.gtfsRouteRouteShortName}</span> | ||
| {t('line')} : | ||
| <span> | ||
| {siriRide ? ( |
There was a problem hiding this comment.
| {siriRide ? ( |
now this check isn't needed
There was a problem hiding this comment.
I left it like this because the whole file is written in this style, but maybe I'll remove it from the whole file since it's not needed inside the returned JSX. sounds good?
There was a problem hiding this comment.
sounds great! let's keep things minimal wherever it's possible
There was a problem hiding this comment.
(feel free to hit the merge button if you want to)
|
I have removed redundant checks. |
|
Is there something else from my side? From what I understand only repository owners can merge to the main branch. right, or should I do something else? |
|
you're right, I thought you already have permission |
Added Router's Link as line name to the Maps' tooltip.
The Link sends you the the line profile page.
@NoamGaash