Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

make marker popups work better #659

Merged
merged 4 commits into from
Feb 25, 2014
Merged

make marker popups work better #659

merged 4 commits into from
Feb 25, 2014

Conversation

samanpwbb
Copy link
Contributor

  • Added overflow-auto and max height to popups
  • Fixed overly broad and aggressive rule intended for tiles that was interfering with image styles in popups
  • Made base type styles smaller

From:
screen shot 2014-01-28 at 3 26 18 pm

To:
screen shot 2014-01-28 at 3 26 02 pm

@tmcw
Copy link
Contributor

tmcw commented Jan 28, 2014

Hm, looks good but enforcing a max-height in CSS may be problematic: Leaflet writes its styles from JS-land and users with custom maxHeight values will see changed behavior. /cc @mourner

@samanpwbb
Copy link
Contributor Author

I fixed this on mapbox.com side as a short term solution: https://github.com/mapbox/mapbox.github.com/commit/0444bf3076031a3d7633c628d61094719c0169ca

@mourner
Copy link
Member

mourner commented Jan 28, 2014

@samanpwbb why not just use maxHeight option of Leaflet?

@samanpwbb
Copy link
Contributor Author

I'd like this sort of thing to be handled by CSS.

I'm guessing Leaflet's maxHeight sets an inline attribute on the element? In that case, maxHeight will override the CSS rule, so I don't think having this in the distributed CSS file is a problem.

@@ -746,9 +757,9 @@
/* Browser Fixes
------------------------------------------------------- */
/* Map is broken in FF if you have max-width: 100% on tiles */
.leaflet-container img { max-width:none!important; }
.leaflet-container img.leaflet-tile { max-width:none; }
Copy link
Member

Choose a reason for hiding this comment

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

The reason why I had to put important here is that some stupid CSS frameworks like Bootstrap use !important too, which will break Leaflet maps unless overriden with !important from Leaflet side. E.g. https://github.com/twbs/bootstrap/blob/master/dist/css/bootstrap.css#L222
So I'd suggest not touching these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the silliest rule I've ever seen. As much as I'd like to step out of this CSS specificity arms race, I don't see how your override could hurt as long as it applies only to tiles.

Copy link
Member

Choose a reason for hiding this comment

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

Right. I should be more specific in Leaflet as well.

@mourner
Copy link
Member

mourner commented Jan 28, 2014

One reason it was not entirely handled in CSS is that I wanted to add a special class when popup reaches max height so that I could style the scrolled container, e.g. add borders like Leaflet does now.

@samanpwbb
Copy link
Contributor Author

Okay, reverted to importants. Now that the rule applies to img.leaflet-tile rather than img this shouldn't interfere with too much.

@samanpwbb
Copy link
Contributor Author

Removed the max height rule with bc3b5fe so this should now be good to go. Sounds like it's best to leave max height management to the front end.

@samanpwbb
Copy link
Contributor Author

@tmcw / @mourner I think this should be fine now but I await approval.


.leaflet-container img {
margin-bottom: 10px;
}
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be more specific?

@mourner
Copy link
Member

mourner commented Jan 29, 2014

Otherwise looks good.

@tmcw
Copy link
Contributor

tmcw commented Feb 4, 2014

Y, we should not style all .leaflet-container img with a margin-bottom.

@samanpwbb
Copy link
Contributor Author

Cool, I made the style more specific. I am ok totally cutting it as well if you guys still feel weird about it. I think it's ok to be a little opinionated with styles within the marker-description though - this is in 95% of the cases just going to make markers look better for people. The other 5% are people who are already messing with popup appearance and then it's a one css line override.

@mourner
Copy link
Member

mourner commented Feb 6, 2014

Yeah, looks good now.

tmcw added a commit that referenced this pull request Feb 25, 2014
make marker popups work better
@tmcw tmcw merged commit 43531cc into master Feb 25, 2014
@jfirebaugh jfirebaugh deleted the smaller-fonts branch March 19, 2014 19:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants