improve hr styles #1685

tlylt · 2021-11-01T00:59:30Z

What is the purpose of this pull request?

Overview of changes:
Fixes #897

Add additional attributes and classes to modify the style, thickness, and color of a horizontal line.

Available style classes:

dotted
dashed
double
Available size classes:

thick
thick-1
thick-2
thick-3
Available color classes (use any of the available BootStrap border color classes):

border-info
border-primary
etc
(edited)

The above is also included in the user guide here

Anything you'd like to highlight / discuss:

Modifications were made for markbind.css, not sure if the frontend bundles (packages/core-web/dist/css/markbind.min.css) need to be updated.

Testing instructions:
Run npm run test in packages/cli

Proposed commit message: (wrap lines at 72 characters)
Give easy ways to add different types of horizontal lines

No convenient syntax is available to style the hr element.

Let's add some helper attributes for the hr element to
improve its style.

Users may use a combination of the available attributes and
BootStrap border-color classes to customize the look of
the hr element.

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes
Removed an unrelated but extra space while editing the site.json file

ang-zeyu

Nice work @tlylt

I think the styling looks good.

Syntax wise, just want to make sure we are on the right track with using attributes instead of .classes.

Pros of css classes:

It's a little more consistent with how we use {.xml} {.java} for inline code currently, although, some other areas like code blocks use highlight-lines for "style" attributes as well (but then again it uses dots in .no-line-numbers for the same feature 😢).
Using . also sticks with the ".class-selectors convention" (if its even a problem for the author, though) for these sort of styles
- on the flip side we are using markdown-it-attrs, which isn't exactly conventional either, so I think we can be a bit looser with attributes here =P

The downsides would be:

exposing some implementation details to the user of course (its a css class)
one extra fugly dot .

I think this is a good tipping point to decide whether to pivot toward . or attributes for things inside { ... }; I'm honestly not sure which is better.

Think we get some user input from @damithc on which would be more consistent / fitting with the syntaxes available right now as a whole too.

ang-zeyu · 2021-11-01T16:44:19Z

docs/userGuide/syntax/horizontalrules.mbdf

+
+<include src="codeAndOutput.md" boilerplate >
+<variable name="highlightStyle">markdown</variable>
+<variable name="code">


Should we split this into the above sections? 10 lines at once might be a bit much to keep track of

Sure, I have split them into individual code blocks under each description (pls see the deployment preview here)

tlylt · 2021-11-02T03:28:00Z

Syntax wise, just want to make sure we are on the right track with using attributes instead of .classes.

Pros of css classes:

It's a little more consistent with how we use {.xml} {.java} for inline code currently, although, some other areas like code blocks use highlight-lines for "style" attributes as well (but then again it uses dots in .no-line-numbers for the same feature 😢).

Using . also sticks with the ".class-selectors convention" (if its even a problem for the author, though) for these sort of styles

on the flip side we are using markdown-it-attrs, which isn't exactly conventional either, so I think we can be a bit looser with attributes here =P

The downsides would be:

exposing some implementation details to the user of course (its a css class)

one extra fugly dot .

I think this is a good tipping point to decide whether to pivot toward . or attributes for things inside { ... }; I'm honestly not sure which is better.

Thanks for the great point on this. I was initially going with the CSS class selector path but was faced with the issue that the hr element somehow does not work with multiple classes. Hence, there is a problem with a combination of style classes(for hr). I moved to attributes since it's 1)shorter, 2)easier on the implementation. The downside as mentioned perhaps might be that users may get confused with whether they need a dot(or not) for such inline styling. I don't really have a strong opinion for this...

Think we get some user input from @damithc on which would be more consistent / fitting with the syntaxes available right now as a whole too.

Yup sure.

damithc · 2021-11-02T05:44:04Z

Thanks for the great point on this. I was initially going with the CSS class selector path but was faced with the issue that the hr element somehow does not work with multiple classes. Hence, there is a problem with a combination of style classes(for hr). I moved to attributes since it's 1)shorter, 2)easier on the implementation. The downside as mentioned perhaps might be that users may get confused with whether they need a dot(or not) for such inline styling. I don't really have a strong opinion for this...

Think we get some user input from @damithc on which would be more consistent / fitting with the syntaxes available right now as a whole too.

I'm OK with the syntax. As a user, it's easier if I don't have a mix of class and attribute for the same element but seems there is no choice due to the issue mentioned? In the user documentation, describe carefully to reduce user confusion. e.g., first describe the attributes and then explain how to add classes too.

BTW, currently limited to only one class per <hr> right? If yes, seems rather awkward to explain that to the user. What is the feasibility of solving that issue first, perhaps around December break? If there is a reasonable possibility, this PR can be put on hold till then.

tlylt · 2021-11-02T13:21:47Z

I'm OK with the syntax. As a user, it's easier if I don't have a mix of class and attribute for the same element but seems there is no choice due to the issue mentioned? In the user documentation, describe carefully to reduce user confusion. e.g., first describe the attributes and then explain how to add classes too.

BTW, currently limited to only one class per <hr> right? If yes, seems rather awkward to explain that to the user. What is the feasibility of solving that issue first, perhaps around December break? If there is a reasonable possibility, this PR can be put on hold till then.

So far I have only discovered that the hr element does not work with multiple classes. Agree that we can put this PR on hold before myself (or others) have a proper look at resolving the underlying issue.

The mix of attributes and style classes can be confusing when users want to customize a hr element. Let's fix the error of multiple classes on hr element by updating the markdown-it-attrs package version and change the syntax from `dotted` to `.dotted` etc. This helps when users need to also customize the color of the hr element, e.g. `--- {.dotted .border-primary}`. The `markdown-it-attrs-nunjucks.js` is also updated with the latest changes from `markdown-it-attrs`.

tlylt · 2022-01-13T08:22:41Z

Hi @ang-zeyu (or other senior devs). I have updated this PR to make the syntax change from style attributes to style classes by:

updating the markdown-it-attrs version so that the upstream changes are propagated
changing the CSS rules from targeting e.g. hr[dotted] to hr.dotted
updating the documentation and test cases

I decided to update markdown-it-attrs here instead of in PR #1701 because markdown-it version update is not really required for this. Also, this way that PR can focus on just the markdown-it package.

This PR description has also been edited with the updated screenshots and links. Let me know if further changes are required.
Thank you!

ang-zeyu · 2022-01-14T15:08:55Z

Lgtm locally!

I've pushed a personal long overdue follow up to a revamp PR that missed removing the markdown-it-attrs / nunjucks patch as well; Sorry for the confusion.
There are also some conflicts from the other PR updating markdown-it dependencies.

tlylt · 2022-01-15T04:25:58Z

I've pushed a personal long overdue follow up to a revamp PR that missed removing the markdown-it-attrs / nunjucks patch as well; Sorry for the confusion. There are also some conflicts from the other PR updating markdown-it dependencies.

Yup, I was a bit unsure why the markdown-it-attrs-nunjucks file suddenly went missing 😂. Other than that I think I have resolved the conflicts in this PR.

add hr styles

b8e579f

ang-zeyu reviewed Nov 1, 2021

View reviewed changes

separate into individual example blocks

0fa5e8a

tlylt changed the title ~~improve hr styles~~ [WIP] improve hr styles Dec 28, 2021

tlylt added 3 commits January 13, 2022 15:51

Merge branch 'master' into update-hr

21e59f8

Update test file due to MarkBind version update

1e2d659

tlylt changed the title ~~[WIP] improve hr styles~~ improve hr styles Jan 13, 2022

Merge branch 'master' into update-hr

ffdde0f

ang-zeyu approved these changes Jan 15, 2022

View reviewed changes

ang-zeyu added this to the 4.0 milestone Jan 15, 2022

ang-zeyu merged commit ed2f323 into MarkBind:master Jan 15, 2022

jonahtanjz modified the milestones: 4.0, 3.1.1 Jan 22, 2022

ang-zeyu mentioned this pull request Mar 29, 2022

Support basic charts #1663

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve hr styles #1685

improve hr styles #1685

tlylt commented Nov 1, 2021 •

edited

Loading

ang-zeyu left a comment •

edited

Loading

ang-zeyu Nov 1, 2021

tlylt Nov 2, 2021

tlylt commented Nov 2, 2021

damithc commented Nov 2, 2021

tlylt commented Nov 2, 2021

tlylt commented Jan 13, 2022

ang-zeyu commented Jan 14, 2022

tlylt commented Jan 15, 2022

improve hr styles #1685

improve hr styles #1685

Conversation

tlylt commented Nov 1, 2021 • edited Loading

ang-zeyu left a comment • edited Loading

Choose a reason for hiding this comment

ang-zeyu Nov 1, 2021

Choose a reason for hiding this comment

tlylt Nov 2, 2021

Choose a reason for hiding this comment

tlylt commented Nov 2, 2021

damithc commented Nov 2, 2021

tlylt commented Nov 2, 2021

tlylt commented Jan 13, 2022

ang-zeyu commented Jan 14, 2022

tlylt commented Jan 15, 2022

tlylt commented Nov 1, 2021 •

edited

Loading

ang-zeyu left a comment •

edited

Loading