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

fix: no styling applied to h1-headings in markdown (closes #1668) #1743

Merged
merged 1 commit into from Oct 10, 2023

Conversation

mschmidm
Copy link
Contributor

@mschmidm mschmidm commented Oct 9, 2023

This fixes #1668 about h1-markdown-headings not being styled.

Bug Explanation

The reason why h1 elements weren't styled but h2 elements were is that h2 got its styling from nextcloud/server/core/css/apps.scss:60-68 which does not contain h1 styles, as the comment explains:

/* BASE STYLING ------------------------------------------------------------ */
// no h1 allowed since h1 = logo
h2 {
    font-weight: bold;
    font-size: 20px;
    margin-bottom: 12px;
    line-height: 30px;
    color: var(--color-text-light);
}

As a fix, I added a custom h1 styling to markdownOutput.scss. I just used the styling from the form title.

Results

Now this:

image

Becomes this:

image

@Chartman123
Copy link
Collaborator

Hi @mschmidm,

thank you for your contribution. 👍🏻 I will do a quick review.

@Chartman123
Copy link
Collaborator

@mschmidm DCO doesn't pass as your signed-of-by doesn't match your configured values in your git commit... (see https://github.com/nextcloud/forms/pull/1743/checks?check_run_id=17542451176)

This should be fixed on your end so that this check will also pass (see dcoapp/app#114 (comment))

Other than that, the PR looks fine :)

@Chartman123 Chartman123 added bug Something isn't working design Related to the design 3. to review Waiting for reviews labels Oct 9, 2023
@Chartman123 Chartman123 added this to the 3.4 milestone Oct 9, 2023
…1668)

Signed-off-by: Michael Schmidmaier <mschmidm@users.noreply.github.com>
@mschmidm
Copy link
Contributor Author

mschmidm commented Oct 9, 2023

Ah okay, the DCO should be fixed now. Thank you! :)

@susnux
Copy link
Collaborator

susnux commented Oct 9, 2023

Ok, but should we sanitize the markdown output to move h1 and h2 to h3 and h4 (and others as well)?
I mean h1 and h2 will break the page layout.

@Chartman123
Copy link
Collaborator

I mean h1 and h2 will break the page layout.

@susnux in which case? Could you please post a screenshot?

@mschmidm
Copy link
Contributor Author

Ok, but should we sanitize the markdown output to move h1 and h2 to h3 and h4 (and others as well)? I mean h1 and h2 will break the page layout.

Seems reasonable, but I think this is different issue.

@Chartman123 I think susnux means that it is currently possible (and likely) two have an incorrect heading order. The form title is a h2. If the user now adds a h1 in the description, there is a wrong order, which is discouraged due to accessibility.
This is the source code of my previous example: A h1 after a h2:
image

@susnux
Copy link
Collaborator

susnux commented Oct 10, 2023

Lets first merge this and then fix the heading orders for descriptions later on

@susnux susnux merged commit c88001b into nextcloud:main Oct 10, 2023
22 checks passed
@mschmidm mschmidm deleted the fix/no-h1-markdown-styling branch October 10, 2023 15:34
@Chartman123
Copy link
Collaborator

Yes because in the end we couldn't make sure that a description of a question that is semantically below a description of a form mustn't contain a header with a smaller number... And what to do with a h6 header then... It's a difficult topic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working design Related to the design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Markdown H1 is not formatted correctly
3 participants