Added Boxes Theme SCSS #586

bluestrat404 · 2022-12-28T04:32:28Z

Hi Chris,

Here's a first stab at adding the theme scss. I'm not sold on "Boxes" as the title, but it works for now. I also included the theme preview on the dev preview page.

This doesn't yet draw the boxes like in that example file on the other thread, but it's ready to — see lines 20 and 29 of boxes.scss.

To get those boxes, we'd need to get the renderer to wrap sections in divs and give them class names like what I'm proposing here in those lines: cmSection and cmChorusSection.

What do you think?

Thanks,
Adam

Added Boxes theme scss and included a Boxes theme preview on the dev preview page.

coveralls · 2022-12-28T06:17:58Z

coverage: 100.0%. remained the same
when pulling d740e85 on bluestrat404:Add-Boxed-Theme
into 6520c32 on no-chris:master.

no-chris

Since it's a print-focused theme, what about calling it simply print2?

I can see how wrapping the sections would be useful for theming, it should be easy enough. I'm just a bit afraid that breaking the rule that the root container's children are lines might break some unit tests that relies on this assumption.

As for the classes name I would suggest using a generic cmSection for all sections and then a cmSection-xxx where xxx is the section label itself, e.g. cmSection-intro, cmSection-chorus, etc.

.gitignore

no-chris · 2022-12-28T07:26:15Z

packages/chord-mark-themes/scss/themes/boxes.scss

+@import './common';
+
+.cmTheme-boxes {
+	@page {


I'm not sure about this. With the other print theme, margins are managed directly by Chord Charts Studio and not the theme itself.

I think handling it in Studio probably makes more sense. If we need a margin slider there, it could be added.

packages/chord-mark-themes/yarn.lock

no-chris · 2022-12-28T07:51:03Z

This is how I would approach the section wrapping.

most of the changes would happen here:

chord-mark/packages/chord-mark/src/renderer/components/renderSong.js

Line 207 in 40879e2

function renderAllLines() {
here you can open the section wrapping when a section label line is detected, and also close it if needed

It's not the most elegant way but I think the most straightforward

Removed additions from local build.

Undoing changes from local build.

Fully reverted now — should've done this automatically not manually.

Updated theme name to Print 2 — and updating the other print to Print 1 to conform to the Dark # naming scheme. And reverted all changes from building locally. Also removed page margin from theme, as that can be handled elsewhere.

…rd-mark into Add-Boxed-Theme

bluestrat404 · 2022-12-28T12:54:00Z

Sorry for spamming commits there. Can you tell I'm a bit of a Github newb?? The final commit has all the changes from your review here — I think.

I like your suggestions as to cmSection.

Thanks!

bluestrat404 · 2022-12-28T13:33:24Z

This is how I would approach the section wrapping.

most of the changes would happen here:

chord-mark/packages/chord-mark/src/renderer/components/renderSong.js

Line 207 in 40879e2

function renderAllLines() {

here you can open the section wrapping when a section label line is detected, and also close it if needed

It's not the most elegant way but I think the most straightforward

This would be the most straightforward, but since each line is a <p> I can't think of how to do what's needed here, which would be to wrap all the <p class="cmLine">s of a section within <div class="cmSection">...</div>.

That is, the below wouldn't work as HTML:

<p class="cmLine"><div class="cmSection">...</p>
<p class="cmLine">...</p>
<p class="cmLine">...</div></p>

What we need is:

<div class="cmSection">
  <p class="cmLine">...</p>
  <p class="cmLine">...</p>
  <p class="cmLine">...</p>
</div>

My next thought is that we may need a function "renderAllSections()" which calls renderAllLines(), instead of renderAllLines() being used directly by customRenderer/songTpl.

Doing it this way, while more of a pain, should be better for writing tests, too, right? Let me know if I'm overcomplicating it.

Adam

no-chris · 2022-12-28T15:07:35Z

packages/chord-mark-themes/scss/themes/print1.scss

@@ -1,7 +1,7 @@
 @import '../abstract';
 @import './common';

-.cmTheme-print {
+.cmTheme-print1 {


this is a breaking change as all client code using this theme would have to update. I'm not aware of any application beside Chord Chart Studio but maybe it's better to avoid it?
Although I'm thinking that in any case, the markup change will also be a breaking change...

packages/chord-mark-themes/yarn.lock

no-chris · 2022-12-28T15:16:23Z

orry for spamming commits there. Can you tell I'm a bit of a Github newb?? The final commit has all the changes from your review here — I think.

You are doing great no worries 💪🏻

My next thought is that we may need a function "renderAllSections()" which calls renderAllLines(), instead of renderAllLines() being used directly by customRenderer/songTpl.

That would be the more elegant way of doing it indeed, but I think more complex. Feel free to give it a go if you feel like it.

For my suggestion, I was thinking that as soon as you have a section label line, then you should have an opening tag too, so the section label template would be updated. It could also include the closure of the previous section if this needs to be, by passing a parameter to the template. Then how to close the last section I'm not sure. Maybe it could be done systematically? So even if a section is not explicitly declared on the first line, we would create a cmSection wrapping for it, meaning that the markup would always contain a section. Well, I'm not sure and I don't have all the details sorted out in my head but I think it could be managed with the current loop.

Oh right I get it, the line wrapper is actually done after, forgot about that 🤦🏻

no-chris · 2022-12-28T15:42:54Z

Grouping lines by sections is something I already did for the ChordPro converter:

chord-mark/packages/chord-mark-converters/src/converters/chordMark2ChordPro.js

Line 67 in 40879e2

const getAllSections = (allLines) => {

I would still try to avoid going there in the main rendering function

Maybe renderLine() could be adapted. That feels like a hack but I'm afraid of the hidden complexity of going the other route

no-chris · 2022-12-28T15:49:32Z

For example:

const wrapperClasses = ['cmSection', 'cmSection-intro'];
return renderLine(rendered, {
	isFromSectionMultiply: line.isFromSectionMultiply,
	isFromAutoRepeatChords: line.isFromAutoRepeatChords,
	isFromChordLineRepeater: line.isFromChordLineRepeater,
	isFromSectionCopy: line.isFromSectionCopy,
        shouldOpenWrapper: true,
        wrapperClasses,
        shouldCloseWrapper: true,
});

parameters could use better names

Changed theme "Print 1" back to "Print", and removed empty line at end of yarn.lock.

bluestrat404 · 2022-12-28T19:20:51Z

Thanks for your graciousness and thoughts. I've committed a fix for the yarn.lock and to roll "print1" back to "print".

I like your idea to hack renderLine rather than introduce a bunch of extraneous new logic. I'll work on that next.

This change wraps sections in <div>s for theme styling. This could be done in a few ways — this way adds some simple logic to renderAllLines in renderSong.js (and related files). The print2.css theme is also updated here slightly.

bluestrat404 · 2023-01-01T20:09:16Z

Chris,

Here's one way to do it. Let me know your thoughts. And, happy new year!

-Adam

no-chris

Happy new year! Thanks for that, I've added a few comments

packages/chord-mark/src/renderer/components/renderSong.js

packages/chord-mark/src/renderer/components/tpl/song.js

packages/chord-mark/src/renderer/components/tpl/line.js

Addressing review comments here: 1. Created function getSectionWrapperClasses 2. Added logic to renderAllLines (and associated) so as not to spawn a needless div. 3. Eliminated repetition in tpl/line.js

no-chris

Nice! Expect some nitpicking in the comments but this looks good.

Now 2 extra challenges 😅

I've updated the master branch this morning so you will need to pull the changes locally in your branch (git pull origin master)
the new feature needs to be covered by unit tests. Luckily most assertions are made against strings and not the html markup so hopefully this should not break a lot of tests. We need at least to update:
- the renderLine() unit test to check that the proper markup is created based on the given parameters
- the renderSong() unit test to check that the main rendering loop is properly filling those parameters

Unfortunately the bulk of the unit test suite is not checking the html markup because most of the rendering logic can be assessed by checking at the rendered text nodes, so I don't have any good example to share on how to test the markup itself. Let me know if you need help

packages/chord-mark/src/renderer/components/renderSong.js

no-chris · 2023-01-03T07:49:41Z

packages/chord-mark/src/renderer/components/renderSong.js

+					shouldOpenSection: opensSection,
+					sectionClasses: sectionWrapperClasses,
+					shouldCloseSection: closePriorSection,
+					closesFinalSection: shouldCloseFinalSection,


We can greatly simplify this by aligning variable names with parameter names:

{ ... shouldOpenSection, shouldCloseSection, shouldCloseFinalSection, sectionWrapperClasses, }

and actually I'm thinking that we may be able to simplify further by removing some local variables

{ ... shouldOpenSection, shouldCloseSection: shouldOpenSection && lineIsInASection, shouldCloseFinalSection: isLastLine(allLines, i) && lineIsInASection, sectionWrapperClasses, }

Nice. I think the problem with ( shouldOpenSection && lineIsInASection ) for shouldClosePriorSection is that we need the last iteration's value of lineIsInASection. That was my thought with the shouldClosePriorSection variable — effectively to store whatever that lineIsInASection value was before we change it now that we know we're in a section.

packages/chord-mark/src/renderer/components/tpl/line.js

Simplifying code based on review. Unit tests will come next.

Added unit tests to check that renderLine creates the proper markup based on the given parameters.

Adding one more test case: a final line that is its own final section.

Better of the description of the final test.

Slight tweak (again) to renderLine test case name. Added test "Wrap Sections in Divs > correctly create and class sections" to renderSong.

bluestrat404 · 2023-01-08T03:23:04Z

It turns out it hadn't broken any tests, so that's good. My latest commit adds a test for renderLine() using regex to check the markup. That may be more strict than needed or wanted, but it works, and I'm proud of it as the first test I've ever written. I'm sure it can be improved on.

It also adds the test for renderSong().

Let me know.

Fixed a typo: sectionWrapperClasses should've been an array length 2, not a string with a space in it.

no-chris

It turns out it hadn't broken any tests, so that's good.

I'm not sure whether that's a good thing or not, but at least it does make things easier 😅

My latest commit adds a test for renderLine() using regex to check the markup. That may be more strict than needed or wanted, but it works, and I'm proud of it as the first test I've ever written. I'm sure it can be improved on.

Very nice for a first test, you should be proud indeed! 👏🏻 I have a few comments for improvement.

It also adds the test for renderSong().

👍🏻

packages/chord-mark/tests/unit/renderer/components/renderLine.spec.js

packages/chord-mark/tests/unit/renderer/components/renderSong.spec.js

Removed unnecessary/dangerous logic. I.e., made the tests appropriately dumber.

no-chris

This looks good, congrats! 👏🏻
I'll merge this in the next few days

packages/chord-mark/src/renderer/components/renderLine.js

packages/chord-mark/tests/unit/renderer/components/renderSong.spec.js

Minor simplification to renderLine. In renderSong, add test to ensure no extra markup is created if no sections are given.

bluestrat404 · 2023-01-10T18:14:42Z

Thanks for all the help and feedback! I look forward to getting the second print theme option going in Chord Chart Studio. I've started looking through the code.

no-chris · 2023-01-10T22:23:55Z

Thanks for your work, this is a nice improvement to ChordMark!

no-chris · 2023-01-11T17:32:15Z

Oh, I forgot, the classes list needs to be updated in ChordMark's Readme. No need to detail all possible sections, cmSection-xxx is fine there too

Include "cmSection" and "cmSection-xxx" classes.

no-chris · 2023-01-12T07:40:31Z

The build is failing because of 2 eslint errors
run yarn run lint (or npm run lint depending on which one you are using) from the root directory to see which ones. For the one related to allLines, just remove the parameter as the function already has access to allLines from where it is declared.

also please run prettier (you might also be able to configure your IDE to automate this) with yarn run format

Thanks!

no-chris · 2023-01-12T07:43:09Z

Also, to avoid merge confliicts, you don't need to commit the bundles:
packages/chord-mark/lib/chord-mark.js.map
packages/chord-mark/lib/chord-mark.js

Bundling is typically done before releasing as a separate step

Fixed lint errors and ran prettier.

…rd-mark into Add-Boxed-Theme

bluestrat404 · 2023-01-12T15:40:13Z

Thanks! I've fixed the errors and ran prettier. Do I need to roll back the bundle changes now, or just not commit them in the future?

no-chris · 2023-01-12T22:26:34Z

No need to rollback the changes I've resolved the conflict

no-chris · 2023-01-13T15:14:42Z

Sorry I keep on adding new things 😅

There are some new styles that I've introduced with a recent feature you may, if you want, add them to your style as well:

chord-mark/packages/chord-mark-themes/scss/themes/print.scss

Line 40 in 6520c32

.cmSubBeatGroupOpener,

Add cmSubBeatGroup opener and closer opacity change.

no-chris · 2023-01-14T16:30:50Z

Merged! I released it as a beta until the rest of the functionality is fully implemented in Chord Chart Studio: https://github.com/no-chris/chord-mark/releases/tag/v0.13.0-beta.0

Added Boxes Theme SCSS

0a36d1d

Added Boxes theme scss and included a Boxes theme preview on the dev preview page.

no-chris requested changes Dec 28, 2022

View reviewed changes

bluestrat404 added 5 commits December 28, 2022 07:28

Reverting .gitignore

891dabb

Removed additions from local build.

Revert yarn.lock

30394aa

Undoing changes from local build.

Update yarn.lock

74ef7c5

Fully reverted now — should've done this automatically not manually.

Update Theme Name and Revert .lock changes

84638a2

Updated theme name to Print 2 — and updating the other print to Print 1 to conform to the Dark # naming scheme. And reverted all changes from building locally. Also removed page margin from theme, as that can be handled elsewhere.

Merge branch 'Add-Boxed-Theme' of https://github.com/bluestrat404/cho…

866a1a8

…rd-mark into Add-Boxed-Theme

no-chris reviewed Dec 28, 2022

View reviewed changes

Rollback Breaking Theme Name Change and yarn.lock

9204883

Changed theme "Print 1" back to "Print", and removed empty line at end of yarn.lock.

Renderer Change - Wrap Sections in Div

8db4411

This change wraps sections in <div>s for theme styling. This could be done in a few ways — this way adds some simple logic to renderAllLines in renderSong.js (and related files). The print2.css theme is also updated here slightly.

no-chris requested changes Jan 2, 2023

View reviewed changes

packages/chord-mark/src/renderer/components/renderSong.js Outdated Show resolved Hide resolved

packages/chord-mark/src/renderer/components/tpl/song.js Outdated Show resolved Hide resolved

packages/chord-mark/src/renderer/components/tpl/line.js Outdated Show resolved Hide resolved

Adding Logic to Remove Extra DIV + small refactors

65dd69a

Addressing review comments here: 1. Created function getSectionWrapperClasses 2. Added logic to renderAllLines (and associated) so as not to spawn a needless div. 3. Eliminated repetition in tpl/line.js

no-chris reviewed Jan 3, 2023

View reviewed changes

bluestrat404 added 5 commits January 3, 2023 09:38

Refactoring and trimming

9e05323

Simplifying code based on review. Unit tests will come next.

Update renderLine.spec.js

8e99229

Added unit tests to check that renderLine creates the proper markup based on the given parameters.

Update renderLine.spec.js

c19afae

Adding one more test case: a final line that is its own final section.

Update renderLine.spec.js

07ac7e4

Better of the description of the final test.

Update renderLine and renderSong tests

80294fe

Slight tweak (again) to renderLine test case name. Added test "Wrap Sections in Divs > correctly create and class sections" to renderSong.

Update renderLine.spec.js

1638c86

Fixed a typo: sectionWrapperClasses should've been an array length 2, not a string with a space in it.

no-chris reviewed Jan 8, 2023

View reviewed changes

Update renderSong and renderLine tests

5d654b2

Removed unnecessary/dangerous logic. I.e., made the tests appropriately dumber.

no-chris previously approved these changes Jan 10, 2023

View reviewed changes

packages/chord-mark/src/renderer/components/renderLine.js Outdated Show resolved Hide resolved

packages/chord-mark/tests/unit/renderer/components/renderSong.spec.js Show resolved Hide resolved

Update renderline.js and renderSong.spec.js

2680dac

Minor simplification to renderLine. In renderSong, add test to ensure no extra markup is created if no sections are given.

bluestrat404 dismissed no-chris’s stale review via 2680dac January 10, 2023 18:13

bluestrat404 and others added 2 commits January 11, 2023 16:08

Update README.md

b4cf638

Include "cmSection" and "cmSection-xxx" classes.

Merge branch 'master' into Add-Boxed-Theme

08f8d87

no-chris self-assigned this Jan 12, 2023

no-chris added the breaking Breaking changes label Jan 12, 2023

bluestrat404 added 2 commits January 12, 2023 10:37

Linting and prettier

b9f6a09

Fixed lint errors and ran prettier.

Merge branch 'Add-Boxed-Theme' of https://github.com/bluestrat404/cho…

cb3e64e

…rd-mark into Add-Boxed-Theme

Update print2.css

d740e85

Add cmSubBeatGroup opener and closer opacity change.

no-chris merged commit 392b8e3 into no-chris:master Jan 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Boxes Theme SCSS #586

Added Boxes Theme SCSS #586

bluestrat404 commented Dec 28, 2022

coveralls commented Dec 28, 2022 •

edited

Loading

no-chris left a comment •

edited

Loading

no-chris Dec 28, 2022

bluestrat404 Dec 28, 2022

no-chris commented Dec 28, 2022

bluestrat404 commented Dec 28, 2022

bluestrat404 commented Dec 28, 2022 •

edited

Loading

no-chris Dec 28, 2022

no-chris commented Dec 28, 2022 •

edited

Loading

no-chris commented Dec 28, 2022

no-chris commented Dec 28, 2022 •

edited

Loading

bluestrat404 commented Dec 28, 2022

bluestrat404 commented Jan 1, 2023

no-chris left a comment

no-chris left a comment

no-chris Jan 3, 2023

bluestrat404 Jan 3, 2023

bluestrat404 commented Jan 8, 2023

no-chris left a comment

no-chris left a comment

bluestrat404 commented Jan 10, 2023

no-chris commented Jan 10, 2023

no-chris commented Jan 11, 2023

no-chris commented Jan 12, 2023

no-chris commented Jan 12, 2023

bluestrat404 commented Jan 12, 2023

no-chris commented Jan 12, 2023

no-chris commented Jan 13, 2023

no-chris commented Jan 14, 2023

Added Boxes Theme SCSS #586

Added Boxes Theme SCSS #586

Conversation

bluestrat404 commented Dec 28, 2022

coveralls commented Dec 28, 2022 • edited Loading

no-chris left a comment • edited Loading

Choose a reason for hiding this comment

no-chris Dec 28, 2022

Choose a reason for hiding this comment

bluestrat404 Dec 28, 2022

Choose a reason for hiding this comment

no-chris commented Dec 28, 2022

bluestrat404 commented Dec 28, 2022

bluestrat404 commented Dec 28, 2022 • edited Loading

no-chris Dec 28, 2022

Choose a reason for hiding this comment

no-chris commented Dec 28, 2022 • edited Loading

no-chris commented Dec 28, 2022

no-chris commented Dec 28, 2022 • edited Loading

bluestrat404 commented Dec 28, 2022

bluestrat404 commented Jan 1, 2023

no-chris left a comment

Choose a reason for hiding this comment

no-chris left a comment

Choose a reason for hiding this comment

no-chris Jan 3, 2023

Choose a reason for hiding this comment

bluestrat404 Jan 3, 2023

Choose a reason for hiding this comment

bluestrat404 commented Jan 8, 2023

no-chris left a comment

Choose a reason for hiding this comment

no-chris left a comment

Choose a reason for hiding this comment

bluestrat404 commented Jan 10, 2023

no-chris commented Jan 10, 2023

no-chris commented Jan 11, 2023

no-chris commented Jan 12, 2023

no-chris commented Jan 12, 2023

bluestrat404 commented Jan 12, 2023

no-chris commented Jan 12, 2023

no-chris commented Jan 13, 2023

no-chris commented Jan 14, 2023

coveralls commented Dec 28, 2022 •

edited

Loading

no-chris left a comment •

edited

Loading

bluestrat404 commented Dec 28, 2022 •

edited

Loading

no-chris commented Dec 28, 2022 •

edited

Loading

no-chris commented Dec 28, 2022 •

edited

Loading