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

"Flag" and "Map" placeholder hints #261

Merged
merged 1 commit into from
Apr 8, 2020

Conversation

ukanuk
Copy link
Contributor

@ukanuk ukanuk commented Apr 7, 2020

Fix #152

Someone with an iPhone ought to double-check dark mode still works properly with this, as I changed code from PR #173 to fix an AnkiDroid regression introduced with fb7f4e8.

In addition, note I used https://commons.wikimedia.org/wiki/File:World_location_map_(W3).svg to get a proper outline of a Winkel III projection as found in this deck's inset maps, but don't think it's needed in sources.csv as I remade the file by hand to optimize size (original outline was 700 nodes, I remade by hand with just 6 nodes).

Lastly, I just noticed in CONTRIBUTING.md that maps get a width of 500px, unlike flags which get a height of 250px. I will rectify this in a bit.

@ukanuk ukanuk changed the title *Flag* and *Map* placeholder hints "Flag" and "Map" placeholder hints Apr 7, 2020
@aplaice
Copy link
Collaborator

aplaice commented Apr 7, 2020

Unfortunately, .night_mode is not recognised in older versions of Anki (2.1.17) — which is a mild inconvenience for end users, as it's easy to upgrade (and besides the "dark mode" in that version of Anki is half-baked anyway) — or of AnkiMobile (2.0.48) — which is a greater issue since newer versions of AnkiMobile are not available if you have a sufficiently old iOS device (I don't really use my old iOS device, only for occasionally testing like here, but there may well be people who do). I don't have access to a newer iOS device / a newer version of AnkiMobile.

I'd suggest including both classes (.night_mode ... and .nightMode ...), in both places.

Otherwise, I really like this! :)


In addition, note I used https://commons.wikimedia.org/wiki/File:World_location_map_(W3).svg to get a proper outline of a Winkel III projection as found in this deck's inset maps, but don't think it's needed in sources.csv as I remade the file by hand to optimize size (original outline was 700 nodes, I remade by hand with just 6 nodes).

There's definitely no need for attribution — the outline of Winkel III projection with standard parameters is definitely not copyrightable.

@ukanuk
Copy link
Contributor Author

ukanuk commented Apr 7, 2020

@aplaice great point, it never occurred to me that old iOS devices might be locked to old versions of the app.

These last two commits are potentially controversial, and I'm not especially pleased that hr alphabetizes to the very bottom and .night_mode css all appears in the middle. But now with a lot of extra CSS compared to a month ago and the fact that one has to scroll to see everything regardless, alphabetizing it all seemed reasonable to me.

@aplaice
Copy link
Collaborator

aplaice commented Apr 7, 2020

Thanks for fixing the night mode issue! (The CSS's a bit verbose, now, but there's no going around it.)

These last two commits are potentially controversial, and I'm not especially pleased that hr alphabetizes to the very bottom and .night_mode css all appears in the middle. But now with a lot of extra CSS compared to a month ago and the fact that one has to scroll to see everything regardless, alphabetizing it all seemed reasonable to me.

It might be worth splitting out into its own issue or PR.

Copy link
Collaborator

@axelboc axelboc left a comment

Choose a reason for hiding this comment

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

The SVGs are very concise, it's nice!

Now that the CSS is growing a little, I wonder whether it would be less confusing to merge classes value and image. They refer to the same element after all: the one that holds the important value (country, capital, map or flag).

I'm pretty sure that if we replace every instance of image with value in the templates and the CSS, the blurring and the SVG placeholders will still work. value sets a font size, which won't affect the images and placeholders. The only thing that image brings is a slightly smaller top margin than value, so we could just add a class called value--image, which would be consistent with the renamed value--front and value--back.

Does that sound like a good idea and are you up for making this change while you're at it, @ukanuk?

A couple other minor changes:

  • How about image__placeholder (or value__placeholder) instead of image__inline-svg for a bit more semantic?
  • Just being picky here, but you seem to have re-ordered some of the rulesets. I typically try to follow the order of the HTML elements (card, then type, then value/image, then info).

src/templates/Country - Map.html Outdated Show resolved Hide resolved
src/style.css Show resolved Hide resolved
@axelboc
Copy link
Collaborator

axelboc commented Apr 7, 2020

Just read your comment about alphabetising the selectors. I get it now, but can't say I'm a huge fan because ordering in CSS matters. Let me know if the changes I suggested make things more readable overall.

@ukanuk
Copy link
Contributor Author

ukanuk commented Apr 8, 2020

I've reverted the alphabetization to follow the order of the HTML elements, and I've also moved the night_mode stuff to the end. Great idea on using currentColor to combine all the night_mode selectors into one, I didn't know about that trick!

  • How about image__placeholder (or value__placeholder) instead of image__inline-svg for a bit more semantic?

My idea was that it ought to be .image svg to match .image img, but I renamed it to .image__inline-svg to avoid confusion about whether it applies to <img> elements referencing an SVG file. But image__placeholder works fine, too, I've switched it.

Now that the CSS is growing a little, I wonder whether it would be less confusing to merge classes value and image.

I think this would work and am happy to make the change, but it involves all templates and makes it harder to review the flag/map placeholder changes. So would it be better to put in a separate PR? This other PR could also provide space to discuss CSS organization as @aplaice suggested. At the very least, I won't merge any commits concerning this until you're able to review my changes addressing the rest of your feedback.

@ukanuk ukanuk requested a review from axelboc April 8, 2020 01:46
Copy link
Collaborator

@axelboc axelboc left a comment

Choose a reason for hiding this comment

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

Very nice, thank you! Yeah, let's do the other refacto in another PR, you're right.

@axelboc axelboc merged commit f39bad8 into anki-geo:master Apr 8, 2020
@ukanuk ukanuk deleted the answer-type-hints branch April 8, 2020 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Visual answer type hints
3 participants