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

feat(layout): Semantic html tags #1626

Open
wants to merge 7 commits into
base: v4
Choose a base branch
from

Conversation

vanadium23
Copy link

Implemented comment from #1588 (comment).

  • Move header out of beforeBody
  • Move center to main
  • Move sidebar to aside
  • Move Date to time
  • Move explorer to nav

Copy link
Collaborator

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

first pass, but still have few items up for discussion

quartz/components/Header.tsx Outdated Show resolved Hide resolved
quartz/components/Header.tsx Outdated Show resolved Hide resolved
quartz/components/renderPage.tsx Show resolved Hide resolved
quartz/components/renderPage.tsx Outdated Show resolved Hide resolved
quartz/components/renderPage.tsx Show resolved Hide resolved
Copy link
Collaborator

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

second pass. cc @saberzero1 @jackyzha0

quartz.layout.ts Outdated Show resolved Hide resolved
quartz/components/ContentMeta.tsx Outdated Show resolved Hide resolved
quartz/components/Explorer.tsx Outdated Show resolved Hide resolved
@aarnphm
Copy link
Collaborator

aarnphm commented Dec 1, 2024

Actually, sorry I might be wrong about section here

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/section

Seems like section should always followed with a heading -> p, so section for each sidebar or container doesn't make sense afaict

@saberzero1
Copy link
Collaborator

Actually, sorry I might be wrong about section here

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/section

Seems like section should always followed with a heading -> p, so section for each sidebar or container doesn't make sense afaict

Sections should be used for parts of a text. Basically every part that the Table of Contents component identifies.

I also believe we should use aside for the sidebars, not section.

Additionally, we should consider the search tag for the Search component.

https://www.w3schools.com/tags/tag_search.asp

@vanadium23
Copy link
Author

@aarnphm @saberzero1 I made a breaking changes in b2423a3, are we ok with it?

@aarnphm
Copy link
Collaborator

aarnphm commented Dec 1, 2024

yea, we might want to bump the version to v4.5 or sth to signify breakage

quartz.layout.ts Outdated Show resolved Hide resolved
@@ -18,7 +18,7 @@ export default ((userOpts?: Partial<SearchOptions>) => {
const opts = { ...defaultOptions, ...userOpts }
const searchPlaceholder = i18n(cfg.locale).components.search.searchBarPlaceholder
return (
<div class={classNames(displayClass, "search")}>
<search class={classNames(displayClass, "search")}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the default container should be div, and diff search-container to tag

        <search id="search-container">
          <form id="search-space">
            <input
              autocomplete="off"
              id="search-bar"
              name="search"
              type="text"
              aria-label={searchPlaceholder}
              placeholder={searchPlaceholder}
            />
          </form>
          <output id="search-layout" data-preview={opts.enablePreview}></output>
        </search>

might have to update scss as well

Copy link
Author

Choose a reason for hiding this comment

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

This example demonstrates potential DOM content when dynamically including JavaScript search functionality in a web application. When search functionality is implemented entirely with JavaScript, if no form is submitted, neither a

element nor a submit is required. For semantics, the element is included to contain the search and filtering capabilities.

(c) https://developer.mozilla.org/en-US/docs/Web/HTML/Element/search#web_app_search

Copy link
Collaborator

Choose a reason for hiding this comment

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

sg, can you then also update the search-container to section and output and form accordingly?

Copy link
Author

Choose a reason for hiding this comment

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

no, you have a js search and no form. MDN specifies for such case, that form does not required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The element semantically identifies the purpose of the element's contents as having search or filtering capabilities. The search or filtering functionality can be for the website or application, the current web page or document, or the entire Internet or subsection thereof.

From their example

<search>
  <label>
    Find and filter your query
    <input type="search" id="query" />
  </label>
  <label>
    <input type="checkbox" id="exact-only" />
    Exact matches only
  </label>

  <section>
    <h3>Results:</h3>
    <ul id="results">
      <!-- search result content -->
    </ul>
    <output id="no-results">
      <!-- no results content -->
    </output>
  </section>
</search>

Ig we don't need form, but for the layout seems to align with their example

@@ -4,7 +4,7 @@ import clipboardStyle from "./styles/clipboard.scss"
import { QuartzComponent, QuartzComponentConstructor, QuartzComponentProps } from "./types"

const Body: QuartzComponent = ({ children }: QuartzComponentProps) => {
return <div id="quartz-body">{children}</div>
return <main id="quartz-body">{children}</main>
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think main should be for quartz-root, and this can be either a section or a div

Copy link
Author

Choose a reason for hiding this comment

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

No,

The content inside the

element should be unique to the document. It should not contain any content that is repeated across documents such as sidebars, navigation links, copyright information, site logos, and search forms.

So I have place it around Content.

* aside --> section
* header --> header + pageHeader
* move Explorer to menu
* move Search to search
* move sidebars to aside
* make header above all content
* make beforeBody --> pageHeader
@@ -27,5 +27,5 @@ export function formatDate(d: Date, locale: ValidLocale = "en-US"): string {
}

export function Date({ date, locale }: Props) {
return <time datetime={date.toISOString()}>{formatDate(date, locale)}</time>
Copy link
Collaborator

Choose a reason for hiding this comment

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

bad merge

@@ -18,7 +18,7 @@ export default ((userOpts?: Partial<SearchOptions>) => {
const opts = { ...defaultOptions, ...userOpts }
const searchPlaceholder = i18n(cfg.locale).components.search.searchBarPlaceholder
return (
<div class={classNames(displayClass, "search")}>
<search class={classNames(displayClass, "search")}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The element semantically identifies the purpose of the element's contents as having search or filtering capabilities. The search or filtering functionality can be for the website or application, the current web page or document, or the entire Internet or subsection thereof.

From their example

<search>
  <label>
    Find and filter your query
    <input type="search" id="query" />
  </label>
  <label>
    <input type="checkbox" id="exact-only" />
    Exact matches only
  </label>

  <section>
    <h3>Results:</h3>
    <ul id="results">
      <!-- search result content -->
    </ul>
    <output id="no-results">
      <!-- no results content -->
    </output>
  </section>
</search>

Ig we don't need form, but for the layout seems to align with their example

<div class="popover-hint">
{beforeBody.map((BodyComponent) => (
<BodyComponent {...componentData} />
))}
</div>
</div>
<Content {...componentData} />
<main>
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's just keep content as article as before wrapping main here is redudant.

The <article> HTML element represents a self-contained composition in a document, page, application, or site, which is intended to be independently distributable or reusable (e.g., in syndication). Examples include: a forum post, a magazine or newspaper article, or a blog entry, a product card, a user-submitted comment, an interactive widget or gadget, or any other independent item of content.

The <main> HTML element represents the dominant content of the of a document. The main content area consists of content that is directly related to or expands upon the central topic of a document, or the central functionality of an application.

In this case article represents the content translated from the markdown, and main encapsulate Body component

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants