UI v3.0 Header + sim container layout tweaks #1996
Conversation
kayla-glick
requested review from
jimmyt857 and
Kruhlmann
and removed request for
jimmyt857
| itemFragment.innerHTML = ` | ||
| <li class="${showInRaidSim ? '' : 'within-raid-sim-hide'}"> | ||
| <a | ||
| href="javascript:void(0)" |
There was a problem hiding this comment.
If we don't intend to use this element as an actual anchor (by having, what I assume to be, an empty link), would it make sense to replace it with a different type of DOM element?
There was a problem hiding this comment.
For accessibility, interactive elements should always use either anchors or buttons (oh and also details elements exist in HTML 5). Since buttons get styled by default, anchors are usually the go-to
|
|
||
| // Config for displaying a warning to the user whenever a condition is met. | ||
| interface SimWarning { | ||
| updateOn: TypedEvent<any>, |
There was a problem hiding this comment.
Is it possible to annotate this listener? Alternatively We could:
interface SimWarning<UpdateListenerType> {
updateOn: TypedEvent<UpdateListerType>,
}There was a problem hiding this comment.
I can try that, yeah. Again just a copy over
| let downloadBinary = downloadFragment.children[0] as HTMLElement; | ||
|
|
||
| if (document.location.href.includes("localhost")) { | ||
| fetch(document.location.protocol + "//" + document.location.host + "/version").then(resp => { |
There was a problem hiding this comment.
Could this not be replaced with:
| fetch(document.location.protocol + "//" + document.location.host + "/version").then(resp => { | |
| fetch("/version").then(resp => { |
There was a problem hiding this comment.
Probably? I just copied this over from the old location. Didn't want to mess with it too much. I'll try it out
| }); | ||
| } else { | ||
| downloadBinary.setAttribute('data-bs-title', 'Download simulator for faster simulating') | ||
| new Tooltip(downloadBinary); |
There was a problem hiding this comment.
I personally don't feel too good about having a constructor with side-effects, but if that's how bootstrap does it, then we should just keep it IMO.
There was a problem hiding this comment.
Yeah unfortunately Bootstrap does a pass when the page loads, but because everything is rendered in via JS we have to manually initialize like this.
| } | ||
| } | ||
|
|
||
| private addSimOptionsLink() { |
There was a problem hiding this comment.
These social link methods seem very similar. Could we abstract some of it away? Maybe into a separate class/classes as this one is getting fairly big.
There was a problem hiding this comment.
Yeah I definitely could, similar to the addToolbarItem method
| } | ||
| } | ||
|
|
||
| let headerFragment = document.createElement('fragment'); |
There was a problem hiding this comment.
These global variables should probably be part of the class in some way, or moved to a separate class.
There was a problem hiding this comment.
I can't seem to find a way to move this into the class. The way the components are set up isn't really conducive to it. You can't initialize it as a property outside the constructor, you can't make it a method because you can't use this before calling super (and you have to pass the element to super to use the root element)
| declare var tippy: any; | ||
| declare var pako: any; | ||
|
|
||
| const URLMAXLEN = 2048; | ||
| const noticeText = ''; | ||
| //const noticeText = 'We are looking for help migrating our sims to Wrath of the Lich King. If you\'d like to participate in a fun side project working with an open-source community please <a href="https://discord.gg/jJMPr9JWwx" target="_blank">join our discord!</a>'; | ||
| // const noticeText = "We're looking for help migrating our sims to Wrath of the Lich King. If you\'d like to participate in a fun side project working with an open-source community, please <a href='https://discord.gg/jJMPr9JWwx' target='_blank'>join our Discord!</a>"; |
There was a problem hiding this comment.
I think we should delete this notice text "backup". If we want to retrieve it, it'll be available in the git history.
Restyling and reworking the header component to match the proposed v3 mock-ups.
Reference image
Screenshots
Desktop
Notice banner
Tablet
Mobile