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.

Sign up for GitHub

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

Jump to bottom

Update hoverText for Locations & Dungeons, and add styling for hoverText icons #433

Merged
merged 1 commit into from
Jan 13, 2020
Merged

Update hoverText for Locations & Dungeons, and add styling for hoverText icons #433

merged 1 commit into from
Jan 13, 2020

Conversation

jhelwig
Copy link
Member

@jhelwig jhelwig commented Jan 3, 2020

No description provided.

@jhelwig jhelwig requested a review from jtappa January 3, 2020 05:53
@jhelwig
Copy link
Member Author

jhelwig commented Jan 3, 2020

This should cover the rest of #425

jtappa
jtappa reviewed Jan 7, 2020
View reviewed changes
logic/poi_locations.yaml
@@ -110,7 +110,7 @@
top: 0
logic: westOfMire
- name: DW Death Mountain
hoverText: DW Death Mountain
hoverText: "DW Death Mountain (2) : Don't need <img src='/static/image/moonpearl.png' class='mini'>"
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add double quotes on this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

The colon in the hoverText confuses YAML parsers if the string isn't quoted.

jtappa
jtappa reviewed Jan 7, 2020
View reviewed changes
logic/poi_locations.yaml
@@ -420,7 +420,7 @@
top: 0
logic: checkerboardCave
- name: Hammers
hoverText: Hammers
hoverText: <img src='/static/image/hammer.png' class='mini'><img src='/static/image/hammer.png' class='mini'><img src='/static/image/hammer.png' class='mini'><img src='/static/image/hammer.png' class='mini'><img src='/static/image/hammer.png' class='mini'><img src='/static/image/hammer.png' class='mini'><img src='/static/image/hammer.png' class='mini'><img src='/static/image/hammer.png' class='mini'>!!!!!!!!
Copy link
Member

Choose a reason for hiding this comment

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

so many hammers

Copy link
Member Author

Choose a reason for hiding this comment

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

One for every peg you have to smack with the hammer in that location. 😄

jtappa
jtappa reviewed Jan 7, 2020
View reviewed changes
logic/poi_locations.yaml
@@ -60,7 +60,7 @@
top: 0
logic: tavern
- name: Chicken House
hoverText: Chicken House
hoverText: Chicken House <img src='/static/image/bomb.png' class='mini'>
Copy link
Member

Choose a reason for hiding this comment

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

is there no way to make these more dynamic so the html gets injected and you're not typing out the same thing for the bomb image over and over here in the yaml? seems not super ideal, especially if for some reason there was a directory structure or naming convention change down the line

jtappa
jtappa requested changes Jan 7, 2020
View reviewed changes
Copy link
Member

@jtappa jtappa left a comment

Choose a reason for hiding this comment

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

Mostly just wondering about the two items that have double quotes around them and the inconsistency there, otherwise lgtm

Adding some kind of dynamic way to inject the images would be a nice to have, wouldn't necessarily have to happen in this PR.

@jtappa
Copy link
Member

jtappa commented Jan 7, 2020

irt the dynamic images would also be in line with issue #20 customizing layout later down in the line, something you could toggle perhaps?

@jhelwig
Copy link
Member Author

jhelwig commented Jan 8, 2020

Adding some kind of dynamic way to inject the images would be a nice to have, wouldn't necessarily have to happen in this PR.

Some kind of templating to reduce the amount of repetition for using images in the hover text would definitely make that easier.

@jhelwig
Copy link
Member Author

jhelwig commented Jan 8, 2020

irt the dynamic images would also be in line with issue #20 customizing layout later down in the line, something you could toggle perhaps?

The kind of customization I was thinking with #20 was more about the fixed grid that is currently used for the item listing, and the dungeon icons immediately below them (not the ones on the map. The lttp-tracker.firebaseapp.com tracker has a mechanism for customizing which (and how many) item icons show up on each row of the grid, for both the items, and for the dungeon icons.

What kind of dynamic images are you thinking of?

@jtappa jtappa merged commit a244150 into Technosorcery:master Jan 13, 2020
@jhelwig jhelwig deleted the tooltip-updates branch January 14, 2020 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtappa jtappa jtappa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jhelwig @jtappa