chore: add module icon #158
Conversation
|
If I read this correctly, this is basically two changes, right? Would you mind terribly in making this into two different PRs? That makes reviewing and testing a bit easier for me :) If you don't know how to do that easily, don't worry about it, and I can take care of that later today :) |
undefined-moe
force-pushed
the
patch
branch
from
106faad to
5cc704e
Compare
undefined-moe
changed the title
| @@ -49,7 +49,7 @@ export const TreeHeader = (props: { icon?: string; text: string; action?: React. | |||
| export const TreeLeaf = (props: { | |||
| level: number; | |||
| height?: number; | |||
| icon?: IconName; | |||
| icon?: IconName | `http://${string}` | `https://${string}`; | |||
There was a problem hiding this comment.
Minor nitpick: due to the way EVEShip.fit is deployed (via https), it will never be possible to have images on http (you can't load images over http when your site is hosted on https for most browsers). So maybe better to just drop the http:// part, giving an earlier warning to the developer?
There was a problem hiding this comment.
While this is an component library instead of a website, shouldn't assume people always deploy them on secure contexts (although they definitely should).
There was a problem hiding this comment.
Yes, this is a component library, but let me reword:
As it is possible this library is used on a https website, it should never contain images to http, as that would fail for those. Having https images for http websites however isn't an issue. In other words: only allowing https means it works for every type of deployment.
That is why I suggest to drop the http:// part, just to make developers more aware not to use http, as that will fail for all sites using this component deployed on https :) It is about protecting future-us from making silly mistakes :)
|
I do like this change, but I also noticed it is not what the in-game GUI does. And I tried to mimic that as close as possible. So now I have a choice ... does ESF goes its own way, or do we stay close to the in-game GUI? As you left the description empty, what made you make this change? Can you convince me one way or the other? :D |
No description provided.