471 - add official link icon #514
Conversation
JuanCaicedo
requested review from
akgupta89,
ckrajewski and
exxonvaldez
as code owners
JuanCaicedo
force-pushed
the
jcaicedo/421-links-to-official-route-pages-breadcrumbs
branch
from
a0172cb
to
5d35b07
Compare
frontend/src/screens/RouteScreen.jsx
Outdated
| position: 'relative', | ||
| top: '5px' |
There was a problem hiding this comment.
I'm not really sure what's going on here, but adding the icon svg leads to some weird alignments 😅
It causes the whole line to grow to 33px from the original 28px.
And also the a tag doesn't seem to wrap completely around the icon, which sits higher than the link box
This makes the icon not aligned with the text next to it.
Thought I tried to figure all this out, it seemed easier to just visually center the icon 😅
|
One UX question: does that link icon go to the same place as the blue link? My initial expectation as a user is that they both go to the same place. If they don't, you may want to change the icon to make it more obvious that it's a different link. Perhaps changing it to a circled (I) would make it clearer. |
|
We merged #508 so you can rebase this :) |
|
Looks pretty good, thanks for doing this! I found that using the SVG icons with text was pretty difficult also, and the relative position "trick" seems like the best way to go that I've found. I noticed that the breadcrumbs are just slightly out of alignment vertically, I think it's because the icon makes the first breadcrumb taller than the others. Not sure what the best fix is for this.
|
|
Agree with @hathix - with this UI, I would expect that clicking either the icon or the adjacent link will open a page in a new window. It would make more sense to me to have a text link that says something like "Official Site" next to the "open in new window" icon. This would probably be too much to display within the breadcrumb text itself, but could be floating on the right side of the white bar. |
There was a problem hiding this comment.
Hey Juan,
Everything looks good, but you'll want to address the UX concerns Jesse and I had. I think the easiest way out is to turn the share icon into an "i" in a circle. Or you can put the "route info" section on the right side of the white bar in a badge or something.
JuanCaicedo
force-pushed
the
jcaicedo/421-links-to-official-route-pages-breadcrumbs
branch
from
8bed38a
to
698a5a0
Compare
|
I made a few changes here 😀
|
| display: 'flex', | ||
| justifyContent: 'space-between', |
There was a problem hiding this comment.
This looks a little weird on mobile
I think solving it could be as simple as adding a media query that sets flexDirection: column for mobile sizes, but I'm not sure how to do media queries within Material UI 😅 The strategy I found in their docs seems complicated, which makes me think I might be looking at the wrong approach
There was a problem hiding this comment.
Nah, don't overcomplicate it.
The more elegant solution, to me, is to make that text label only visible on medium and large screens (you can do that with tools from here, i think) and replace it with a small icon on small screens.
|
You got a lint error, by the way. See the |
|
Anyway, I think your approach of using the "Official Site" on the right side was great. Much better from a UX perspective. |
Note: This PR builds on #508 and is blocked until that merges. To make reviewing easier, I opened this PR against that one. After #508 merges, I'll change the base branch to master 👍
Motivation
Proposed changes
Screenshot