mergesort / Recap

A What's New screen, and more
MIT License
161 stars 5 forks source link

Added ability to specify vertical alignment for image against text #4

Closed AndyQ closed 6 days ago

AndyQ commented 1 week ago

defaults to existing behaviour - center if not specified or invalid alignment specified

mergesort commented 1 week ago

Hey @AndyQ, I appreciate the enthusiasm and want to say thank you for the contribution! Before merging this in, I had a few questions, and will possibly ask for a few small changes based on .

  1. Could you explain a little bit more about the goal for this change? Are there any specific use cases you have, think are important here, or other reasons you may want to deviate from the standard What's New layout.
  2. Could you provide a few screenshots of how this would look in practice? It's a bit hard to visualize how this would look across scenarios, so I want to make sure there aren't any nuances or quirks I can't tell just by reading the code.
  3. It looks like there's an hAlignment and zAlignment, but both of those properties are always initialized to the same values. Since it doesn't seem to be used, is there a reason we would need an hAlignment property at all?

Thanks again, I look forward to learning more about your work!

AndyQ commented 1 week ago

Hi @mergesort,

  1. So, we wanted the image shown aligned at the top rather than centered (just our preference here really). I allowed alignment also at the bottom just for consistency really.

  2. See attached screenshot - I included two items aligned at the top (which is what we'd use) and one aligned at the bottom.

  3. I did a little playing and these were my findings: The hAlignment is needed to align both the image and the text at the edge specified (within the HStack). For some reason (assuming this is due to the Clear color item though), we also need to set the ZStack set to the same alignment (top, center or bottom). If only the HStack is set to top but the ZStack is center then there is a small amount of padding at the top (the image isn't quite top aligned), and similary at the bottom (which is what the zAlignment is for).

Setting both the hAlignment and zAlighment to the same gives the correct aligment.

Thanks - Andy

recap_image
iKenndac commented 1 week ago

Heck yeah. Wanting the icons top-aligned was literally the first thing that came to mind when I saw the screenshots in the README. I have literally zero authority over this project, but 👍.

iKenndac commented 1 week ago

Now I think about this more, I'd actually want to put this is a view modifier rather than adding "alignment: top" into every item in my release notes (since I'd want it to be consistent). Something like .recapItemSymbolAlignment.

Happy to contribute if that's needed.

mergesort commented 1 week ago

Thanks for all the feedback, I'll do my best to respond to all of the comments!

  1. So, we wanted the image shown aligned at the top rather than centered (just our preference here really). I allowed alignment also at the bottom just for consistency really.
  2. See attached screenshot - I included two items aligned at the top (which is what we'd use) and one aligned at the bottom.

@AndyQ That all makes sense to me, thanks a lot for sharing the screenshots, and thanks for explaining the context to me!

  1. I did a little playing and these were my findings: The hAlignment is needed to align both the image and the text at the edge specified (within the HStack). For some reason (assuming this is due to the Clear color item though), we also need to set the ZStack set to the same alignment (top, center or bottom). If only the HStack is set to top but the ZStack is center then there is a small amount of padding at the top (the image isn't quite top aligned), and similary at the bottom (which is what the zAlignment is for).

Sorry if the question wasn't clear, what I was wondering was why the need for two variables if hAlignment and zAlignment are always going to be set to the same thing. I would think that we could create one property, something like stackAlignment or verticalconAlignment, which would be set to .top, .center, or .bottom.

Now I think about this more, I'd actually want to put this is a view modifier rather than adding "alignment: top" into every item in my release notes (since I'd want it to be consistent). Something like .recapItemSymbolAlignment.

But I think that may be moot, because I agree with @iKenndac's point here, unless you have a reason you'd want to provide a different alignment for each icon @AndyQ?

If we go this route, for naming consistency I would suggest .recapIconAlignment. That would take a parameter of VerticalAlignment, resulting in a function signature that looks like this.

func recapIconAlignment(_ alignment: VerticalAlignment) -> some View

How does that sound to both of you? If that's a reasonable path, I'd definitely appreciate the help implementing this version, and would gladly merge it in now that I understand the use case!

iKenndac commented 6 days ago

The view modifier could be recapIconDefaultAlignment(…) in case a particular item wants to be overridden in the Markdown (i.e., "Why not both?").

I'm happy to do it, but I don't want to steamroll Andy's PR.

AndyQ commented 6 days ago

Nope I'm more than happy to have it as a ViewModifier - I can't see that I'd want different alignments per icon. For info though, the two variables were because the two stack alignments are different types and I didn't find out how to combine these without it being more messy (although I didn't spend much time looking at that though!)

@iKenndac Feel free to change this if you want!

mergesort commented 6 days ago

@AndyQ Oh, of course, they're different types — that completely makes sense! 🤦🏻‍♂️

@iKenndac Let's go the recapIconAlignment ViewModifier only path, I appreciate the great suggestion and look forward to the help. I'll close out this PR to avoid any confusion, but don't take that as a sign that I don't want this functionality. 🙂