Pablo-Gonzalez-Calderon / showybox-package

A Typst package for creating colorful and customizable boxes
MIT License
49 stars 5 forks source link

Add support for more/custom placements of boxed title #13

Closed Andrew15-5 closed 11 months ago

Andrew15-5 commented 11 months ago

In the past, I made this simple box with tcolorbox in LaTeX:

image

In v1.1.0 of the showybox there are only 5 positions, and they all make the middle of the boxed title pinned to the top border. Maybe you can add anchor (boxed-vertical-anchor) argument that will take top, middle, bottom and a custom float or ratio for granular settings.

boxed title will probably receive more parameters in the future anyway, so it would be better to change the config structure for it sooner. Something like:

#showybox(
  title: "title",
  title-style: (
    boxed: false, // Not boxed.
  ),
)[body content]
#showybox(
  title: "title",
  title-style: (
    boxed: true, // Make title boxed with default settings (auto doesn't make sense in pair with false).
  ),
)[body content]
#showybox(
  title: "title",
  title-style: (
    boxed: (
      anchor: ( // Boxed (true) with custom settings.
        vertical: top, // 0.3
        horizontal: start, // 0.7
      ),
    ),
  ),
)[body content]
Pablo-Gonzalez-Calderon commented 11 months ago

Looks interesting 🧐!

Currently I have some free time fot trying to implement this, but also for solving Issue #12. Maybe a refactorization of boxed and title drawing would be necessary.

I will take a look at the code for achieving this, (unless you want to make these changes by yourself 😅).

Andrew15-5 commented 11 months ago

I would love to contribute myself, but I have a lot of things to do. If I have a chance in the future, then sure.

Pablo-Gonzalez-Calderon commented 11 months ago

Oh, ok. No problem 🧐. I will look at the code then. But also I would like to discuss more deep about some of these properties. Could you explain more about anchor properties (what would look like when it's top, middle, bottom).

And also, do you think it'll be better to refactor the properties like this:

#showybox(
  title-style: (
    boxed: true // Or false
    boxed-style: (
      anchor: "middle",
      align: left // Or a float, as discussed in #14 
    )
  )
)[Content]

or like this

#showybox(
  title-style: (
    boxed: true // Or false
  ),
  boxed-style: (
    anchor: "middle",
    align: left // Or a float, as discussed in #14 
  )
)[Content]
Andrew15-5 commented 11 months ago

We can discuss the details later. I think the first config example looks very readable and logical (boxed, boxed-style). But the anchor and align are very confusing. The anchor shows which coordinate of the boxed title should be the origin. By default, (in current version) the (vertical) anchor is equal to center:

image

The example in the issue's message/comment, the anchor: top. So we are judging by the title position relative to the frame's top borderline.

And the alignment is an "alignment" if used with constant positions and more of an "offset" if used with floating number. The namings are temporary, I don't know yet what names are the best for readability and logic.

Pablo-Gonzalez-Calderon commented 11 months ago

I got you. Maybe "alignment" would be bad naming due to we may want to give a float instead. I'll think about it meanwhile I modify the code for boxed titles.

Pablo-Gonzalez-Calderon commented 11 months ago

I finished all the basic stuff for the boxed title's new layout. Here's an example of how it looks now:

image

There're still some issues to handle with the shadow, but the general idea is working. Finally, I decided to set the property values like this at the moment:

#showybox(
  title-style: (
    boxed: true,
  ),
  boxed-style: (
    anchor: (
      y: horizon,
      x: left,
    ),
    offset: 0pt // Only in x direction
  ),
)[Content]

This way there isn't too much dictionaries encapsulation and also keeps coherency with the names and values of all the other properties.

Andrew15-5 commented 11 months ago

Why y: horizon doesn't look centered? It's slightly above than it should be. I'm guessing the reason is 2 lines instead of 1. It looks like the vertical anchor is about 1.5em from the bottom of the title, which means it will only grow above/outside of the main frame. This sounds like a good sub-feature (add starting point for anchor: top, bottom or center for even more flexibility).

Pablo-Gonzalez-Calderon commented 11 months ago

It's not really centered for an aesthetic reason. If the title is too big (like 3 or 4 lines high), there will be too much empty space around the title and inside the body (because it must leave space for the boxed title). But now that I'm thinking about it, changing this behaviour to the expected case (to be really centered) would also depend of fixing Issue #12 because a better method for getting the title's dimensions will be needed in order to really center the title.

On the other side, I don't think an y-offset for the boxed title would be necessary at the moment, because a bigger offset will produce the anchor: top or `anchor: bottom cases`. But also this could fix Issue #12 in an inelegant way (harder to the user to comprehend immediately)

Pablo-Gonzalez-Calderon commented 11 months ago

Copy of last message in #16 for continuing the conversation:

Cool! Now everything is working. But we still need y: horizon to be in the center. We have 2 options:

  • make y: center as a 4th value or
  • replace horizon with `center.

I think the 2nd option is better, since center is more applicable than horizon and it is present in anchor(x). The current horizon behavior can be achieved by anchor: (y: bottom), offset: (y: 0.5em) if the vertical offset is implemented (which is the 2nd and the last thing to do).

Pablo-Gonzalez-Calderon commented 11 months ago

As you mentioned in #16, where talking about swap the top and bottom anchor, I think replacing horizon with center is also the best option, due to readabilty and comprehension that it gives.

On the other hand, for implementing y-offset there's still too much to do (at least in my mind). Let me explain the main issue, as I finally figured out from #12:

  1. For controling mutable title sizes, we have to evaluate their size at the begining of the render of the showybox and at the end of it. Thus, any change of size while rendering will be catched and used properly for setting proper spacings.
  2. To achieve (1), the better option would be using state(). Therefore, an id implementation for the showyboxes would be necessary for storing each showybox's title size information.

With that in mind, now it must be implemented. I have a bad experience using meta functions like styles or state, so unfortunately it'll be kinda painful to me to do the implementation. But finally, with a proper handling of sizes, not only issue #12 will be fixed, also I could get the actual height of the boxed title and center it properly in the y-axis (and therefore implement the y-offset).

I'm going to implement this slowly in the following days, as I have some reports to finish and my "small vacations" are ending 😅

Also, I would like your opinion of releasing v1.2.0 with the current changes, and later create a (tentative version) v1.3.0 with these new changes; or wait until these changes are completed to release version 1.2.0.

Andrew15-5 commented 11 months ago

If we release things as they are, then there will be breaking changes, because we will probably change the config's structure/namings. To avoid breaking changes and more changes overall, the best way is to wait and release when everything is discussed and ready for "production".

Pablo-Gonzalez-Calderon commented 11 months ago

If we release things as they are, then there will be breaking changes, because we will probably change the config's structure/namings. To avoid breaking changes and more changes overall, the best way is to wait and release when everything is discussed and ready for "production".

Hmmm. You're right. Later, changing a name would be a chaos and will force the versioning to jump directly to v2.0.0 without other justification than "names changed".

Well, I'm going to start working in this issue with mutable title sizes, and come back when I have something done 😉

Andrew15-5 commented 11 months ago

About semver. Even though, overall, the package does look production-ready, I think you've jumped to stable release too quickly. Literally:

v0.1.1
v0.2.0
v0.2.1
v1.0.0
v1.1.0

But there is no going back now, so yeah, with breaking changes you would have to change major version. But there are examples of a product that has a big major version (Node.js is on 20th major version now), so it's ok.

Pablo-Gonzalez-Calderon commented 11 months ago

About semver. Even though, overall, the package does look production-ready, I think you've jumped to stable release too quickly. Literally:

v0.1.1
v0.2.0
v0.2.1
v1.0.0
v1.1.0

But there is no going back now, so yeah, with breaking changes you would have to change major version. But there are examples of a product that has a big major version (Node.js is on 20th major version now), so it's ok.

Honestly, at the beginning I thought that this package wouldn't have so many features, so I considered "normal" changes to be enough to qualify as "breaking changes", but suddenly new features were added by other contributors, and everything escalated very fast to know where the limit was 😅 (in addition with that this was my very first package so my expectations were very low).

Andrew15-5 commented 11 months ago

I have a rubby package and I might have overestimated how the things will go (I started at 0.8.0). But since 0.10 isn't always a stable release, I can iterate over the beta versions for as long as I need to. I already have a pretty good, but not complete (and not ideal) package for making flowcharts using cetz. But we have a problem with arrows in the meantime.

Pablo-Gonzalez-Calderon commented 11 months ago

I've made some small steps to finally get the "real" title dimensions. But now I ran out of ideas to solve a problem I have, so if you have any idea of how to solve it, it'll be appreciated.

Context: I've noticed that getting the title dimensions not always can be done using measure and styles. I pre-rendered the title inside a block that emulates the "real" conditions where the title would be; one of the properties that this block have is width, a value given by the user, that's 100% by default. If the width is a length type everything is perfect, but if it's a ratio type everything is horrible: measure doesn't get the real dimensions.

So, in order to solve this ~big and horrible~ little problem, y ran a type-check before calculating the dimensions, so if the width is a length the code uses measure, but if it's not it needs to do more steps. (<- Currently I'm stuck here)

To calculate the estimated lines the title will use, I do the following:

  1. Calculate the container width (the container where the title will be put in later)
  2. Get the given title (as an argument) width -- here it has no length limitation.
  3. Get the "real" space where we can "write" (i.e. container width minus insets)
  4. Divide the width of step (2) by the width of step (3) and approximate it to the greatest integer. This will return the number of lines the final title would have, and it's correct in the majority of the cases.

Here's a portion of the code that does the above:

if type(width) == ratio {
  layout(size => {
    let container-width = size.width * width
    let title-string-size = measure(title, styles)
    [#title-string-size]

    // First get the writable space
    // Note: I cannot substract the insets, because they are in `em`, but this measures are in `pt`, so later Typst won't let me divide
    let writable-space = container-width - showy-value-in-direction(left, props.frame.thickness, 0pt)/2 - showy-value-in-direction(right, props.frame.thickness, 0pt)/2
    [#writable-space]

    // Now, calculate how many lines will fit inside that space
    let lines = calc.ceil(title-string-size.width / writable-space)
    [Lines: #lines]
  })
} else {
  // Pre-rendering "normally" will be effective
  let pre-rendered = block(
    spacing: 0pt,
    width: width,
    fill: yellow, // For recognizing it when it was not hidden
    inset: (x: 1em), // Ignore this, it's for testing some previous thoughts
    showy-title(props, title)
  )

  place(
    top,
    hide(pre-rendered)
  )

  let rendered-size = measure(pre-rendered, styles)
  // Save it into the state
}

That will look like this: image

As you can see, the second showybox has "Lines: 2" which is correct, but the first one no, because it has 2 lines, no 1. Why? Because it cas a "\n" in the title argument 🥹. So, in order to handle all cases, I need to also be able to consider new-line characters, but also considering that a line between new-line chars could be larger than the container dimensions.

So, currently I ran out of ideas. I've been working on this for 3 hours, and now I'm a bit exhausted with no ideas. I won't be able to code much until Saturday/Sunday, so I think it's also a good time to see if we can elaborate a good method for calculating this ~horrible~ complicated dimensions.

Pablo-Gonzalez-Calderon commented 11 months ago

Also, would be great if you know a way em and pt lengths can be divided.

Pablo-Gonzalez-Calderon commented 11 months ago

Hi!

I'm happy to announce it's done! Now the title height is well-calculated and stored in a state. Here's a "real" horizon-centered title: image

The code is up, if you want to test it. Also, Issue #12 was solved with this 🥳

Now, what i have left is:

Andrew15-5 commented 11 months ago

I think offset is broken because it's not y-axis only. Well, if anchor: (y: horizon) (center doesn't work) then offset works on the y-axis, if y: top, then offset doesn't work at all (x left/right has a permanent x-axis offset of a 1em toward the center), and if y: bottom, then offset works on the (-1, 1) vector (both axis):

#import "@local/showybox:1.2.0": showybox

#{
  let title = [My\ title]
  let code = raw("code")
  showybox(
    code,
    title: title,
    title-style: (boxed: true),
    boxed-style: (anchor: (x: left, y: top), offset: -1em),
  )

  showybox(
    code,
    title: title,
    title-style: (boxed: true),
    boxed-style: (anchor: (x: right, y: bottom), offset: 1em),
  )

  showybox(
    code,
    title: title,
    title-style: (boxed: true),
    boxed-style: (anchor: (x: center, y: horizon), offset: 1em),
  )
}

image

Andrew15-5 commented 11 months ago

And the default x position should be center. Otherwise the bottom-left corner doesn't look nice without some tweaking:

#import "@local/showybox:1.2.0": showybox

#{
  let code = raw("code")
  for title in ([My\ title], [My title]) {
    showybox(
      code,
      title: title,
      title-style: (boxed: true),
      boxed-style: (radius: (bottom-left: 0pt, rest: 0.5em)),
    )
  }
}

image

Pablo-Gonzalez-Calderon commented 11 months ago

I think offset is broken because it's not y-axis only. Well, if anchor: (y: horizon) (center doesn't work) then offset works on the y-axis, if y: top, then offset doesn't work at all (x left/right has a permanent x-axis offset of a 1em toward the center), and if y: bottom, then offset works on the (-1, 1) vector (both axis):

#import "@local/showybox:1.2.0": showybox

#{
  let title = [My\ title]
  let code = raw("code")
  showybox(
    code,
    title: title,
    title-style: (boxed: true),
    boxed-style: (anchor: (x: left, y: top), offset: -1em),
  )

  showybox(
    code,
    title: title,
    title-style: (boxed: true),
    boxed-style: (anchor: (x: right, y: bottom), offset: 1em),
  )

  showybox(
    code,
    title: title,
    title-style: (boxed: true),
    boxed-style: (anchor: (x: center, y: horizon), offset: 1em),
  )
}

image

Yeah, as I said, the proper name of offset must change to margin with the current implementation. Internally the given value is an inset value (dictionary or length) for a block that contains the title.

I did this for flexibility. This way, the user can give a dictionary with values like left, right, x, rest, etc. which is less restrictive. Changing this implementation for a "real" offset (not margin) would imply to constraint this dictionary values to only x and y, as the place element that contains the title-boxed-block only can handle those values (dx, dy). I wasn't very sure of which implementation was better, so I preferred to use the "less hardworking" one, to discuss this later. So, it's a possibility to use the other method 🤔

Pablo-Gonzalez-Calderon commented 11 months ago

And the default x position should be center. Otherwise the bottom-left corner doesn't look nice without some tweaking:

#import "@local/showybox:1.2.0": showybox

#{
  let code = raw("code")
  for title in ([My\ title], [My title]) {
    showybox(
      code,
      title: title,
      title-style: (boxed: true),
      boxed-style: (radius: (bottom-left: 0pt, rest: 0.5em)),
    )
  }
}

image

Yeah, I'm working on this. I'm trying to implement a default margin of 1em at both left and right sides so it'll look nicer

Andrew15-5 commented 11 months ago

I don't understand why offset must be changed to margin. I think offset perfectly describes what it would do. If we make anchor: (x: left), then the offset will mean "by how much the title should be moved from its origin (left)". Same with anchor: (x: right). With anchor: (x: center) the meaning is even more simple to grasp: if we "offset" the title from its origin (center) by 1cm to the left, then the offset will be -1cm, and if it's moved to the right, then the offset will be 1cm.

If we are talking in terms of CSS, then we have padding and margin. But we also have left which is exactly what offset is doing with the boxed title in the showybox. Here is an example:

index.html ```html
```

image

In CSS, we can either choose left or right and the direction will change with it (left: 1em moves to the right, but right: 1em moves to the left). In showybox/typst x-axis is constant, so 1em will (should) always move things to the right. In showybox we also have a center which acts the same way. I hope you get the idea. It's not rational to name offset as left/rightcenter, because it's not CSS, and we already have an anchor with these values. And margin in CSS refers to the margin's thickness (not by how much to move an element).

Pablo-Gonzalez-Calderon commented 11 months ago

As I said, I wasn't very sure of which implementation was better, so I preferred to use the "less hardworking" one, to choose the optimal later. It seems, the unimplemented one (offset instead of margin) will be the definitive.

Pablo-Gonzalez-Calderon commented 11 months ago

Now, what i have left is:

  • ~Rename offset property to margin, as it's more comprehensive (offset is more like a one-direction-displacement, while margin is like "how much space have near"; and also the implementation of offset, finally was more like a margin implementation).~ Reimplement offset
  • Change default weight to the title
  • Document all the new files I've made, and refactor some functions to make the code clearer
  • Update the manual using tidy and codelest
  • Take a big glass of cola to celebrate I've finally won against the ~horrible~ state function

Well, now first, second and third point are done. You can check de code to see if it works for you locally. As I mentioned, and you would notice, now by default there's a 1em space around the boxed title in the x-axis, so it'll look nicer with the default properties, and also avoids having a boxed title of the same length of the showybox's body (which is quite unwanted since it vanishes the "floating box" illusion).

Finally, I mention that you can pass a length or dictionary (with x and y keys) to boxed-title.offset. And also the offset in x-axis applies in left-to-right direction, and in y-axis in top-to-bottom direction.

There's some buggy behaviours yet with shadows in ~ugly and not aesthetic~ special showyboxes with extreme properties values, but that's not the point of this issue, so I prefer to ignore them here.

Andrew15-5 commented 11 months ago

Yeah, I've tried the x/y offset — pretty cool stuff. I haven't tried shadows, since personally I'm not a huge fan of the shadow. Well, only if there was a smooth/gradient shadow like in the manual:

image

I only don't like that the shadow don't quite extend to the corner (the shadow is too short near the tip of the corner). But if this kind of shadows (similar to CSS shadows) were available, then you can sign me in. If you want, you can make a separate feature request about this.

Andrew15-5 commented 11 months ago

We also have to talk about this title, title-style, boxed-style thing. boxed-style is for sure isn't great since it should be boxed-title-style, but this is probably too long, and we already have a title-style. So everything kinda points to the fact that title-style and boxed-style must be merged. How exactly? Well, this is the tricky part.

We can do title-style: (boxed: true, boxed-style: ..). This is one of the most simple and readable configs that I can think of. I want to combine boxed and boxed-style, but perhaps this can be confusing, I'm not sure. Like either boxed-style: false (or none) or boxed-style: (:). Doesn't really look confusing to me.

But since everything inside of the title-style already is a style, then maybe boxed-style is too repetitive and we better off with just boxed. What do you think? Is it too soon to discuss this?

Pablo-Gonzalez-Calderon commented 11 months ago

I only don't like that the shadow don't quite extend to the corner (the shadow is too short near the tip of the corner). But if this kind of shadows (similar to CSS shadows) were available, then you can sign me in. If you want, you can make a separate feature request about this.

Yeah, the problem there was that the shadow in reality is an image that's been escalated in arbitrary proportions to be bigger than the code block.

Luckily, using tidy will keep issues like that out of the way 🤔

Pablo-Gonzalez-Calderon commented 11 months ago

We also have to talk about this title, title-style, boxed-style thing. boxed-style is for sure isn't great since it should be boxed-title-style, but this is probably too long, and we already have a title-style. So everything kinda points to the fact that title-style and boxed-style must be merged. How exactly? Well, this is the tricky part.

We can do title-style: (boxed: true, boxed-style: ..). This is one of the most simple and readable configs that I can think of. I want to combine boxed and boxed-style, but perhaps this can be confusing, I'm not sure. Like either boxed-style: false (or none) or boxed-style: (:). Doesn't really look confusing to me.

But since everything inside of the title-style already is a style, then maybe boxed-style is too repetitive and we better off with just boxed. What do you think? Is it too soon to discuss this?

Yeah, I think combining boxed and boxed-style would be a bit of confusing, but I think it's the best option to have things separated. Despite it can be a little bit ambiguous and repetitive having boxed-style inside title-style, I think it's better to keep that name, as it --essentially-- is a new subgroup of style properties, and reading boxed-style: none is more self-explicative that boxed: none; and also boxed-style: (:) makes more sense (as it's a dictionary of styles) than boxed: (:).

The other option I can imagine, and you didn't mention, is just moving all boxed-style properties inside title-style, but with the prefix boxed-. Like title-style( boxed-anchor: (...), boxed-radius: 4pt ). Personally I don't like this option because merges too much two things that are quite different, but it's still an option.

Andrew15-5 commented 11 months ago

The other option I can imagine, and you didn't mention, is just moving all boxed-style properties inside title-style, but with the prefix boxed-.

boxed-this and boxed-that is not pretty. I can only imagine the chaos when boxed title will get more options as the showybox package matures and therefore there will be boxed-even boxed-more boxed-options inside the title-style.

Pablo-Gonzalez-Calderon commented 11 months ago

Yeah, I didn't like that option for that. So, I think the "boxed-style inside title-style" option is the winner. To clarify, this will be the expected options:

title-style: (
  ...,
  // If we DO want boxed-title...
  boxed-style: (
    anchor: ... // As now
    offset: ... // As now
    radius: ... // As now
  ),

  // If we DO NOT want boxed-title...
  boxed-style: none
)

I think that's it. Unless you think a property name needs to be changed for being more comprehensive 🤔

Andrew15-5 commented 11 months ago

I think that's pretty good. I have no complaints. Actually, maybe one. But not about the namings.

Why offset can receive length? Why do we need a horizontal offset? Why not just (x: .., y: ..)?

Pablo-Gonzalez-Calderon commented 11 months ago

Why offset can receive length? Why do we need a horizontal offset? Why not just (x: .., y: ..)?

Hmmm. I think I didn't understand the second question. But offset could receive both length and dictionary for testing purposes. I forgot I did that until now you ask. Surely, I will remove the length acceptance once the final changes are done. Thus, only a dictionary with (x: .., y: ..) will be expected

Andrew15-5 commented 11 months ago

I think I didn't understand the second question.

Oh, sorry, not horizontal offset, I meant diagonal offset.

Andrew15-5 commented 11 months ago

Thus, only a dictionary with (x: .., y: ..) will be expected

Ok, then everything is good.

Pablo-Gonzalez-Calderon commented 11 months ago

I've pushed the discussed changes 🥳

As I briefly tested, everything is working with the refactored properties. Anyways, if you notice something that is not working right, tell me to modify it as soon as possible.

On the meantime, I'm going to close this issue as it seems finished 🧐