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

feat: Add text-transform attribute to the style block #1118

Merged
merged 1 commit into from
Apr 8, 2023

Conversation

alexstoick
Copy link
Contributor

@alexstoick alexstoick commented Mar 31, 2023

This provides an option to override the functionality provided by the
theme, or apply text-transform outside of the theme.

The functionality of the text-transform is as below:

  • text-transform: none - will disable any transformation (like the uppercasing by terminal theme)
  • text-transform: uppercase (uppercase not upper as per your message) - will force all characters into uppercase.
  • text-transform: lowercase - will force all characters into lowercase.
  • text-transform: capitalize - will uppercase the first letter of every word

In addition, this commit introduces:

  • helper methods on the d2graph.Style struct to determine the type of
    text-transform to be applied.
  • ApplyTextTransform method on the d2graph.Attributes which will
    transform the Label.Value to the correct text case.

Screenshots pending.

Fixes: #1117

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

amazing, thanks so much for opening a PR for this.

  1. your intuition is right about the boolean type, but we should make all of them typed in a refactor and test rigorously, vs having a temporary one-off type in the list of styles. (there's an issue for this: compiler: add different scalar types #285). could you use the same Scalar type here please?
  2. Tests:
    name: "terminal",
  • Add a caps lock = false for an object in there
  • Regenerate the test with TESTDATA_ACCEPT=1 ./ci/test.sh ./e2etests -run TestE2E/themes
  • Check the output SVG to see that caps lock=false worked.

And then do that for another test to test capslock=true works.

@alexstoick
Copy link
Contributor Author

alexstoick commented Apr 3, 2023

@alixander - updated as per your comment:

  • removed Boolean class. I kept the helper methods - as I think the duplication would be mad, but if you consider it's not in line with the rest of the style I can remove them 👍
  • added tests & updated description with screenshots. Let me know if you think the other-user entry messes up the vibe of the origami test, and I will setup a whole new one 👍

@alexstoick alexstoick marked this pull request as ready for review April 3, 2023 17:22
@janydoe
Copy link
Contributor

janydoe commented Apr 4, 2023

Maybe it would be better to make style.convertCase: upper (lower, pascal, title, etc)

@alixander
Copy link
Collaborator

alixander commented Apr 5, 2023

@janydoe that's an interesting point, thank you for that suggestion. maybe we literally just do https://developer.mozilla.org/en-US/docs/Web/CSS/text-transform

yeah that's more flexible. i've found myself wanting to do capital casing sometimes, and i think many others would too, and I don't want separate keywords for that. So then the way to disable the theme's uppercasing is to do text-transform: none

Sorry for the moving goal post @alexstoick , would you mind changing to that? You can just support the values upper and none for this one to accomplish the original, and other values can be added later if that's easier

@alexstoick
Copy link
Contributor Author

@alixander I'm happy to implement the text-transform spec. Just to be clear on desired behaviour:

  • text-transform: none - will disable any transformation (like the uppercasing by terminal theme)
  • text-transform: uppercase (uppercase not upper as per your message) - will force all characters into uppercase.
  • text-transform: lowercase - will force all characters into lowercase.
  • text-transform: capitalize - will uppercase the first letter of every word (defined by some separator) - I'll need to check if there is a helper method for this, or I might come back to you with a proposal.

If the above sounds good - LMK and I'll update the PR.

Thank you 🙇

@alixander
Copy link
Collaborator

@alexstoick sounds good to me!

@alexstoick alexstoick changed the title feat: Add caps-lock attribute to the style block feat: Add text-transform attribute to the style block Apr 6, 2023
@alexstoick alexstoick force-pushed the feat/caps-lock-toggable branch 3 times, most recently from be9ddb1 to b7ad89f Compare April 6, 2023 09:45
@alexstoick
Copy link
Contributor Author

@alixander - I've implemented the text-transform as requested; before I go ahead and add final touches to description (screenshots, etc) - let me know if this is 🆗 for you. I've changed a few of the tests to cover all of the text-transform options.

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

generally looks good to me, let's add tests and get this in, ty @alexstoick !

d2graph/d2graph.go Outdated Show resolved Hide resolved
d2graph/d2graph.go Outdated Show resolved Hide resolved
d2graph/d2graph.go Outdated Show resolved Hide resolved
d2graph/style_helper.go Outdated Show resolved Hide resolved
@alexstoick
Copy link
Contributor Author

@alixander - when you say let's add tests - I have already updated some of the tests in e2e/themes; are there more tests that you would like to see on specific components, or new tests all together?

d2graph/d2graph.go Outdated Show resolved Hide resolved
@alixander
Copy link
Collaborator

oh, i guess those tests do cover it all, nvm!

d2graph/d2graph.go Outdated Show resolved Hide resolved
d2graph/d2graph.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

can you add a changelog here too please?

@alexstoick alexstoick force-pushed the feat/caps-lock-toggable branch 2 times, most recently from c529fbc to 120e439 Compare April 7, 2023 22:26
@alixander
Copy link
Collaborator

@alexstoick looks like a format error, if you run ./make.sh fmt it should fix

@alexstoick
Copy link
Contributor Author

@alixander - yep; odd that my formatter didn't auto-fix. Updated now ✅

d2graph/d2graph.go Outdated Show resolved Hide resolved
d2graph/d2graph.go Outdated Show resolved Hide resolved
d2graph/d2graph.go Outdated Show resolved Hide resolved
@alexstoick alexstoick force-pushed the feat/caps-lock-toggable branch 2 times, most recently from 44676e3 to 52977d3 Compare April 8, 2023 17:48
d2graph/d2graph.go Outdated Show resolved Hide resolved
This provides an option to override the functionality provided by the
theme, or apply `text-transform` outside of the theme.

The functionality of the `text-transform` is as below:

- `text-transform: none` - will disable **any** transformation (like the uppercasing by `terminal` theme)
- `text-transform: uppercase` (uppercase not upper as per your message) - will force all characters into uppercase.
- `text-transform: lowercase` - will force all characters into lowercase.
- `text-transform: capitalize` - will uppercase the first letter of every word

In addition, this commit introduces:
- helper methods on the `d2graph.Style` struct to determine the type of
  `text-transform` to be applied.
- `ApplyTextTransform` method on the `d2graph.Attributes` which will
  transform the `Label.Value` to the correct text case.
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

tested and works great, thanks @alexstoick

@alixander alixander merged commit 2ab19ee into terrastruct:master Apr 8, 2023
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.

Feature Request: Make CapsLock toggable for elements
3 participants