Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More examples completed. #189

Merged
merged 37 commits into from
Nov 14, 2019
Merged

More examples completed. #189

merged 37 commits into from
Nov 14, 2019

Conversation

NickFitz
Copy link
Contributor

@NickFitz NickFitz commented Oct 3, 2019

Still a few examples to go, but I'll get these over now so people can check them out (pun intended).

@NickFitz NickFitz requested a review from AmeliaBR October 3, 2019 19:44
@AmeliaBR
Copy link
Member

AmeliaBR commented Oct 9, 2019

Whoops. Somehow didn't get back to this when I did reviews last week.

My only concern is adding framework-specific code to examples.css. When @nchan0154 had issues with the TomTom stylesheets code before, she added in local <style> blocks. But if we have to do that for every example page, it's probably better to create a single CSS file with the required rules — but they should still be correctly scoped. I started a dedicated issue for brainstorming: #193

In the meantime, not sure what's the best strategy. I'd like to merge the rest of this.

@Malvoz
Copy link
Member

Malvoz commented Oct 9, 2019

My only concern is adding framework-specific code to examples.css

I'm also responsible for doing that, to make it easier to update styles as we had many duplicate inline styles and style blocks across multiple example pages. So adding a couple more of those now probably wont hurt, especially taking #193 (comment) into consideration.

@AmeliaBR
Copy link
Member

AmeliaBR commented Oct 9, 2019

to make it easier to update styles as we had many duplicate inline styles and style blocks across multiple example pages

Yeah, I mean, the issue is less about which file to put the CSS in & more about making sure the selectors are sufficiently scoped so there aren't weird interactions & we can easily understand why each rule is there.

@Malvoz
Copy link
Member

Malvoz commented Oct 12, 2019

Oh, I wasn't concerned with which file to put the CSS, but rather why adding inline styles now would be blocking this merge?

I'd like to merge the rest of this.

@@ -0,0 +1,287 @@
<!DOCTYPE html>
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to include this file in this PR?

If so, please link it from the correct part of index.html & change up the commented bits so that the headings are still visible & the unfinished examples use a single block comment.

(to at least keep them contained,
until we figure out a better way to sandbox styles)
@AmeliaBR
Copy link
Member

OK, I pushed my own "good enough" solution to the TomTom styles issue: I moved the icon rules into their own file, that we can link to everywhere we would link to the TomTom's full stylesheet that includes those rules.

@NickFitz, when checking out the PR, I discovered a mostly commented-out example file. Were you intending to commit that yet? If so, can you tidy it up a bit?

@AmeliaBR
Copy link
Member

OK, merging everything. Sorry for the delay. Hoping to find some time/energy to catch up on more substantive reviews, soon.

@AmeliaBR AmeliaBR merged commit b063aa3 into Maps4HTML:master Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants