chore(www): upgrade servers-view to Lit
#1941
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1941 +/- ##
======================================
Coverage 32% 32%
======================================
Files 45 45
Lines 2620 2646 +26
Branches 340 347 +7
======================================
+ Hits 860 873 +13
- Misses 1760 1773 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/www/views/servers_view/index.ts
Outdated
| render() { | ||
| if (this.shouldShowZeroState) { | ||
| return html` | ||
| <button type="button" @click=${this.requestPromptAddServer}> |
There was a problem hiding this comment.
you only really need type="button" in a form afaik
There was a problem hiding this comment.
I believe it's the opposite actually. TIL a missing type means type="submit", which is meant for form submission. See https://html.spec.whatwg.org/multipage/form-elements.html#the-button-element.
There was a problem hiding this comment.
Sure, but we aren't in a form context. It is technically correct I just don't do this usually cuz it reads weird. I'm ambivalent.
src/www/views/servers_view/index.ts
Outdated
| <button type="button" @click=${this.requestPromptAddServer}> | ||
| <server-connection-indicator connection-state="disconnected"></server-connection-indicator> | ||
| <header> | ||
| <h1>${this.localize('server-add')}</h1> |
There was a problem hiding this comment.
h1 should probably be the root "outline" header, maybe? so that would make this h2/h3?
There was a problem hiding this comment.
<h1> here is used explicitly in the context of <header> that's heading sectioned content. It would be odd to have the first h* element inside <header> be <h2>. Although as I write this, I realize I'm actually missing a sectioning element here. Added that.
There was a problem hiding this comment.
"H1 tags are used to indicate the primary topic of your webpage to visitors and search engines." However, we don't really care about SEO here, so I'm ambivalent.
There was a problem hiding this comment.
The primary topic of this page is not "Outline" though, it doesn't help accessibility because all pages would be the same. The "Add server" is the appropriate (though clunky) topic of the page, even setting aside that you can have multiple section headers. The source you link gives a good example and also admits it further down:
Can a page have multiple H1 tags?
[...] In some circumstances, where a page truly covers multiple high-level topics, multiple H1s can be used. Google’s John Muller has stated that it will not penalize pages with multiple H1 tags. [...]
The only argument I could see is that "Add server" shouldn't be a header at all, but just regular content, in which case we should revert to a text node or <p> element. Maybe "Servers" should be the appropriate heading for both the server list and the empty state?
There was a problem hiding this comment.
I agree that "Servers" is a better H1, yeah.
sbruens
force-pushed
the
sbruens/servers-lit
branch
from
1ca893a
to
f8e9447
Compare
sbruens
force-pushed
the
sbruens/servers-lit
branch
from
f8e9447
to
be96024
Compare
* chore(www): upgrade `servers-view` to Lit * Remove use of `paper-button` from `servers-view`. * Use semantic elements for header and footer. * Use `<button>` element. * Change `line-height` values. * Rename `useAltAccessMessage` to `shouldShowAccessKeyWikiLink`. * Hoist `<user-coms-dialog>` to the root element. * Remove unused import. * Add a section element.
No description provided.