Skip to content

Fix unexpected behavior in UG due to invalid HTML markup#1837

Merged
ryoarmanda merged 8 commits into
MarkBind:masterfrom
tlylt:fix-vue-warn
Mar 24, 2022
Merged

Fix unexpected behavior in UG due to invalid HTML markup#1837
ryoarmanda merged 8 commits into
MarkBind:masterfrom
tlylt:fix-vue-warn

Conversation

@tlylt

@tlylt tlylt commented Mar 18, 2022

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Overview of changes:
Fixes #1794
Also fixes #1840 (very much related so putting it together here)

  • Include a 'troubleshooting' section in UG for putting such errors

Anything you'd like to highlight / discuss:
Not too sure why, but sometimes it's fine for block-level elements to be in <span> but sometimes not.
E.g. in embed section, seems like this span tag works fine and does not trigger any vue warning.
image

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Fix unexpected behavior in UG due to invalid HTML markup

Invalid HTML markup may cause Vue hydration issues.

Let's replace span tags with div tags to avoid invalid
inclusion of block-level elements in span tags.

This prevents possible errors in HTML syntax.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@jonahtanjz jonahtanjz requested a review from ryoarmanda March 18, 2022 04:01

@ryoarmanda ryoarmanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting issue 👀 I'm also a bit intrigued on the embed case, maybe the issue is for specific block-level elements like tables? (cc @jonahtanjz) I think we can also change those to use div for posterity sake, what do you think?

The docs you have written up are a good start, maybe some little stylization nits below:

Comment thread docs/userGuide/troubleshooting.md Outdated
@jonahtanjz

Copy link
Copy Markdown
Contributor

I'm also a bit intrigued on the embed case, maybe the issue is for specific block-level elements like tables? (cc @jonahtanjz)

Having block elements within inline elements should not be allowed according to the SSR guide for Vue. For some reason, Vue does not always warn or have issues with some of the cases.

For example, putting the following code on a MarkBind site,

<span>
<div>Test</div>
</span>

doesn't seem to have any warnings.

Similarly, even using the example provided on Vue's page,

<p>
<div>Test</div>
</p>

no warnings as well.

Putting a table in a <span> seems to consistently cause the warning to show up.

The following seems to cause the warning to show up as well,

<p>
<ul>
<li>1</li>
<li>2</li>
</ul>
</p>

I think we can also change those to use div for posterity sake, what do you think?

Although some of the offending components don't result in any warnings, I think they should still be changed as well since they are not valid HTML and may cause some problematic issues in the future.

@jonahtanjz jonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @tlylt!

A couple more invalid HTML:

Also noticed a few of our Vue components having block-level elements in <span>. This can be fixed in another PR.

Popover.vue
MinimalPanel.vue
NestedPanel.vue

@tlylt

tlylt commented Mar 22, 2022

Copy link
Copy Markdown
Contributor Author

A couple more invalid HTML:

Good catch! I have fixed the cases mentioned. I also:

  • added one warning in the reusing content -> includes section to inform users to choose div over span
  • removed some span to div replacement as those spans within a boilerplate appear to be important for the variable to work. Will open a separate issue for this.

@jonahtanjz jonahtanjz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @tlylt! Looks good to me 👍

Let's wait for @ryoarmanda to look through and approve as well :)

@ryoarmanda ryoarmanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me as well! Will merge tonight

@ryoarmanda

ryoarmanda commented Mar 23, 2022

Copy link
Copy Markdown
Contributor

Some weird thing happening here 🤔 the checks have been in queue for a while now, and I didn't see it ever running. Even the checks tab show 0 checks, and no workflow records appear.

If this keeps going, I might have to close and reopen this PR to kick the checks back up again.

@tlylt

tlylt commented Mar 23, 2022

Copy link
Copy Markdown
Contributor Author

Some weird thing happening here 🤔 the checks have been in queue for a while now, and I didn't see it ever running. Even the checks tab show 0 checks, and no workflow records appear.

If this keeps going, I might have to close and reopen this PR to kick the checks back up again.

GitHub is experiencing some operational error, we can probably wait out.

@jonahtanjz

Copy link
Copy Markdown
Contributor

If this keeps going, I might have to close and reopen this PR to kick the checks back up again.

Will need to try this since it is still stuck in queue.

@ryoarmanda ryoarmanda closed this Mar 24, 2022
@ryoarmanda ryoarmanda reopened this Mar 24, 2022
@ryoarmanda

Copy link
Copy Markdown
Contributor

Okay, checks are running now.

@ryoarmanda ryoarmanda merged commit 0e37eaf into MarkBind:master Mar 24, 2022
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.

Typo in UG: Syntax Cheat Sheet Unexpected siteNav scrolling effect on small screens

3 participants