Closed esitarz closed 4 years ago
@esitarz Could you do a master merge on this? I think there's some changes that have come in that will change the look of things a bit.
Interesting! A couple of thoughts:
Also, it's too hard to visually distinguish between the name of a site and the neighborhood. I see the logic of not using smallcaps for neighborhoods because caps is sometimes an accessibility mess, but if they're going to be in the same case then there needs to be contrast created some other way.
Hey hey! Thanks @comeoneileen for taking the time to review!
Font change is more important for performance than it is for styling. A “native font stack” like this one is for optimum text rendering across all devices and OS. The Google fonts give the UI a more polished look, while not deviating too rapidly from the default font used previously.Poppins
on the headings complements the body nicely but if anyone had others for consideration, that would be great!
I see what you mean about the colors and I think adding a top or left border with a more pronounced pop of color would be a great addition that allows people to scan the list easier. Perhaps someone can iterate on these cards once they get in but they are definitely a good step towards standardizing both the sidebar cards and the map cards.
Interesting find on the text overflow. Could you tell me your device/browser info? I was unable to recreate on MacOS Firefox/Chrome. I've tried adding a few declarations that might preemptively fix this-- if you could pull in and confirm again. I'm grasping at thin air a bit without being able to recreate locally.
Good consideration! I think a color change paired with a horizontal line distinguishing the two headings would suffice.
@esitarz I really like the direction of these style updates! I think it makes the list clear and easy to read and goes along well with the other main colors. The addition of color palette and component styling are super helpful too. I have a few suggestions to tighten it up a little bit so the cards don't take up so much screen real estate and so that hovering/scrolling over the cards is a little smoother. The main changes I made were:
Clear Search
buttonlocation-list--item
class back to the cards/list items but remove the styling – (Your changes removed that class from each item (card) in the list but it is used for determining the number of results being displayed so I added that back so the results would show the correct number)This is what it looks like:
This is what hover looks like (again, open to changing this if it's too bright or people want a different color) :
Sorry about the terrible graphics, was originally a quicktime vid and obviously the conversion to gif didn't go too well
Hey @amaxama! Thanks so much for polishing this up. I think your bullets are all very solid-- especially like the subtle background color on the sidebar!
I think the change in color is a bit too extreme and competes too much with the indicator "dot" color (blue/babyblue/red). Maybe --primary-50
or simply a very light grey would be a better hover state choice?
It also looks like those sidebar cards are packed in tight and could use just a tad more vertical spacing.
Maybe margin: .5rem 0;
or so?
Here's a quick mockup of what I was thinking we could add to the cards to give them a little more "pop" of color.
Cool, yeah. Digging the direction this is headed. Thank you both!
I still see overflow (pictures below) on Windows 10/Chrome 84.0.4147.105.
@esitarz Yeah, I totally agree on the hover color...I went back and forth so much eventually just decided to submit, hoping someone would give feedback so thanks! :) The only thing with a light grey is it kind of blends a bit with the background, but I'll see if I can get an okay contrast going. It's just supposed to be a subtle visual cue for the user anyway so something minor is probably fine like you said. .......... I tried primary-50 and it's a bit of the same situation with the secondary purple but with blue..a bit too extreme.. with gray-50 and increasing the shadow a tiny bit on hover I think it's a pretty good mix.
Fair point about the tightness of the cards (it gets a little accentuated by the vertical shift during hover but overall I think that effect is nice so better to just give a litter more space) I think it's at 0.5rem right now but I'll try it with a little extra.
That vertical indicator is a nice touch on the cards! Got that in there... is it overkill with the color?
@comeoneileen Thanks for the feedback! That's definitely an issue with the overflowing text...not exactly sure what the problem is. Eric, do you know what might have caused the overflow?
Here are the updates:
@amaxama WAY COOL! These vertical indicators look great here! Yeah honestly, we might be able to get away with removing the indicator "dot" next to the title now. It's not bad, but the border indicator conveys the color well and having it twice might be a tad overkill...I could go either way, what do you think?
Onto the hover state. I think how you have it here looks good. Not saying this is the right choice but just throwing another suggestion out there: what about bumping the sidebar bg and the card bg down 1-2 shades, then making the hover bg white?
As for that text overflow issue. Were you able to recreate on your end? I added a little css that might address the problem but without being able to recreate on my machine, it's a bit tougher. Let me know if you still see it after pulling in my last commit and if it's still there, maybe we could try paired programming to debug?
@esitarz Yay! Yeah, I'm thinking similarly with it being a tad overkill in both places, but I also could go either way. I'll float it by some other TCMAP people to see if there's input.
My initial hunch with the background darker + white hover is it might be a little odd, but I can totally try it!
For the overflow, I was experiencing it as well. I hadn't actually looked into it at all until now but I think I found a fix. Will post an update later tonight
@esitarz Okay... I tried the darker bg with white shadow.... 1 shade darker:
2 shades darker:
I definitely think it was worth a shot but I'm leaning towards what we had before instead.... What do you think?
here's a visual with the circle status indicators gone but the side colors slightly thicker:
@amaxama yeah I'm with you on reverting the colors to the way you had it.
I'm also sold that the indicator dots can go!
I've got one more idea: what if we use the opacity of the border-right indicator for the hover state?
So drop them all to opacity: .75
or something that looks right, then :active, :hover {opacity: 1};
?
Any luck on that text overflow bug? I'm wondering if it's as simple as changing line 3 in card.css
to this?
.card { flex-flow: column wrap; }
@esitarz After consulting with some other people, everyone preferred to keep the second indicator in the title so I think we'll keep it.
I'll try out the opacity idea!
I did find a fix for the overflow! I mentioned it a few comments back but it was mixed in with everything else so easy to miss.
@esitarz I played around with opacity and got it working. It's pretty subtle because a lot of users are mobile and I didn't want the visual to be too different between mobile and web so it's just a slight intensification on hover.
What do you think is going to be the best way to merge our changes together? We could open a branch off the actual repo then put the changes from your fork into the branch and then the changes from my fork into the branch? Because it's so many changes, I think it would actually be good to have them pushed to a branch off the main repo first so that the CI steps can run.
@amaxama way cool on your above comments. I think it's looking great!
Whatever you think is easiest for the new branch. I'm assuming you've branched off my original, so your local has the "final styling" changes, but it doesn't look like you've pushed to your remote. So you could probably just point to the main repo and push up?
@esitarz Okay! Yeah that sounds good, will do! I'll link this PR into that one so we keep a reference to all this great dialogue!
Merged in with Issue #238
What
Theming:
HSL()
color one time.Styling Architecture Refactor: It will be easier to maintain the CSS if each "component" is broken down into an individual stylesheet. I have began to add new sheets as necessary but the entire
styles.css
could eventually be compartmentalized into this structure.Card Styling: I am creating a set of card styles that can be used on the left sidebar cards as well as the popup cards. Note on this: It would be more DRY to simply reuse the
.card
class instead of overwriting the.mapboxgl-popup-content
class and repeating styles. Perhaps we can tackle this down the road.Adding several new icon assets to the
/public
directory. I am usingicon_search
as the new search icon-- if anyone wants to use the others in the future, they are here!Then doing some general styling revisions within the
styles.css
sheet.Please feel free to comment or slack me directly to chat about anything in here!
Why
UI has room for improvement! Styling Architecture needs help!
Ensure the following interactions work as expected. Please test using a mobile form factor.
<< don't see this one anymore
Check the app in the following web browsers: