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

Bird-a-Day Support #52

Closed
wants to merge 11 commits into from
Closed

Bird-a-Day Support #52

wants to merge 11 commits into from

Conversation

JamesDowney
Copy link
Contributor

@JamesDowney JamesDowney commented Jan 20, 2022

image
The formatting is a bit rough around the edges but it displays the relevant information.

Added a UseLink component similar to EquipLink.

Added a HorizontalRule component which can be used with <Hr />, similar to html's <hr />. It's not the first time it's been mentioned, it's nice to have an easy divider in a tile.

@JamesDowney JamesDowney added enhancement New feature or request IOTM To mark an issue/PR related to IOTM support labels Jan 20, 2022
@JamesDowney JamesDowney self-assigned this Jan 20, 2022
@JamesDowney JamesDowney linked an issue Jan 20, 2022 that may be closed by this pull request
@JamesDowney
Copy link
Contributor Author

JamesDowney commented Jan 20, 2022

I can rename UseLink if it's potentially confusing with the way hooks start with use.

Also thinking more on this, maybe links to equip/use/cast/familiar/whatever should be one smarter function you can pass any $item/$familiar/$skill to, with an optional parameter for hiding said link. And possibly usable within <Line> in addition to <Tile>.

Probably I should take the link for usable items out of this PR.

@docrostov
Copy link
Member

docrostov commented Jan 20, 2022

would 100% rename useLink to something like "buffLink"; to gausie's point on discord earlier, we really do not want to have "useX" functions not be hooks because when we actually start writing contributing.md that will make it even more confusing to people.

love <Hr>; that is lovely and great, will be a huge boon in future tiles.

as for the tile itself, i think this is a bit too much information for the bird tile, but i have trouble figuring out how best to address the problem, because i could see cases where people do actually want the tile to show them the "extra" enchantments (ie, weapon damage % in CS, cold res while doing the peak in an avatar path, etc). this seems like a tailor-made tooltip problem where we can tier the effects and show the "tier 1" effect first and have the others in a tooltip on hoverover of the tier 1 effect.

at the end of the day i think this is fine for a first pass, i just think we should revisit this when we've made some initial tooltip stuff because this seems like a relatively easy tile to make more compact via tooltips for the extraneous enchantments.

@JamesDowney
Copy link
Contributor Author

JamesDowney commented Jan 20, 2022

To clarify, there isn't a function being called with the name useLink, it's pretty much just a file name in the components folder that is capitalized, named UseLink because it provides a link for a usable item. The only place you'd see the name is in imports pretty much. It doesn't feel intrinsically confusing to me when it's alongside EquipLink, but I could be wrong.
It is now UsableLink for clarity.

To the point of information overload for the tile, completely agree. Formatting the buff effects in a sensible way was starting to get to be a pretty big piece of code, so I figured I'd just put the unformatted information out there now and revisit it later. Being able to just add an effect name that when hovered over with the mouse popped up the effect description similar to how it's displayed in the skillz menu in-game would be very neat, but is probably non-trivial.

@JamesDowney JamesDowney deleted the bird-a-day-support branch July 16, 2022 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request IOTM To mark an issue/PR related to IOTM support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bird-A-Day Calendar Support
2 participants