Skip to content

Handle autocloseable tag with trailing text correctly#183

Merged
Shinigami92 merged 6 commits intomainfrom
issue-182
Mar 22, 2021
Merged

Handle autocloseable tag with trailing text correctly#183
Shinigami92 merged 6 commits intomainfrom
issue-182

Conversation

@Shinigami92
Copy link
Copy Markdown
Member

closes #182

@Shinigami92 Shinigami92 added the type: bug Functionality that does not work as intended/expected label Mar 19, 2021
@Shinigami92 Shinigami92 requested a review from ST-DDT March 19, 2021 12:24
@Shinigami92 Shinigami92 self-assigned this Mar 19, 2021
@Shinigami92
Copy link
Copy Markdown
Member Author

Hi @ST-DDT o/ :smile:
Do you have any additional suggestions? Maybe some more test cases?
Or anything I can improve in the code itself?

@ST-DDT
Copy link
Copy Markdown
Contributor

ST-DDT commented Mar 19, 2021

I'm not sure about the test, because it only tests that it stays as it is, but maybe there is nothing else to be tested there.
One thing you might want to test, is text that is composed of paragraphs/linebreaks and continuous sections. Similar to the following html section:

<span>This is a very long sentence so it got
    wrapped automatically at the end of the line.
    <br />
    But some things need to be highlighted beyond the capabilties of a
    single newline, so lets try to use two too:
    <br />
    <br />
    Foobar is the the result of naming a bar foo!
</span>

I will contact you in person to discuss this PR in detail later.


PS: Please fix the build to show warnings like these only once:

You have a misspelled word: autocloseable on String

@Shinigami92
Copy link
Copy Markdown
Member Author

I think your example would look like

span This is a very long sentence so it got
  | wrapped automatically at the end of the line.
  br/
  | But some things need to be highlighted beyond the capabilities of a
  | single newline, so lets try to use two too:
  br/
  br/
  | Foobar is the result of naming a bar foo!

You are indicating in the first sentence that you predict that the line would wrap by my formatter.
This is not the case!
I may consider to implement such a mechanic, but that would be a new issue.
Also it could be really hard, because I don't have to only check if the length is to long, but also if the line can hold more content of the further line and so on. 🤔


The warnings in the Files changes tab are coming from the linter step in the GitHub Actions workflow.
I could add an if to perform linting only on linux + node-lts 🤔
We should move this into a new issue if it really worth the effort

Copy link
Copy Markdown
Contributor

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@Shinigami92
Copy link
Copy Markdown
Member Author

Please fix the build to show warnings like these only once

done ✔️

@Shinigami92 Shinigami92 merged commit 1bcb6bb into main Mar 22, 2021
@Shinigami92 Shinigami92 deleted the issue-182 branch March 22, 2021 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: bug Functionality that does not work as intended/expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Treat the autocloseable tag with trailing text correctly

2 participants