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

Improved deck recap component layout #111

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

piedpiper358
Copy link
Contributor

Change title color to be more readable. Layout now better supports two-line strings.

Before:

2022-08-05_16-57-57

After:
2022-08-11_17-32-08

2022-08-11_17-14-07

@sebastientromp
Copy link
Contributor

I'm fine with the color change, I think it's a good change.
I would still try to align the icons, even if the number of lines on each element is different.

@piedpiper358
Copy link
Contributor Author

I would also like to remove the "margin-left: 40px" from ".best-against"
Because of it, the name of the deck cannot fill the allotted space

2022-08-11_17-52-03

@sebastientromp
Copy link
Contributor

I would also like to remove the "margin-left: 40px" from ".best-against" Because of it, the name of the deck cannot fill the allotted space

2022-08-11_17-52-03

What does it look like if you remove it?

@piedpiper358
Copy link
Contributor Author

piedpiper358 commented Aug 30, 2022

Sorry, didn't see your message. I do it by myself.

2022-08-30_19-41-04
2022-08-30_19-41-32
2022-08-30_19-43-54
2022-08-30_19-45-31

@@ -48,13 +48,14 @@
display: flex;
flex-direction: column;
justify-content: flex-start;
width: 150%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this 150%?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to make the deck title block wider. Without it, it would look like this.

2022-09-06_12-53-56

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand what the issue is:

  • The "deck-summary" element takes the full width
  • It's divided between deck-image, deck-title and best-against
  • deck-image has a fixed width, best-against is set to take 100% of the space

This means that the layout itself has to figure out what's the best way to give room to each widget.
There are multiple solutions:

  • Attribute width to each element so that the total is 100% (you can have a mix of pixel width + flex-grow: 1)
  • Keep the sizes dynamic, using flex-basis to tell the layout the relative size of the items

In this specific case, the component's width will always be constrained by the width of the ads column, so 400px. So I'm fine with using absolute pixel sizes in this case, but the other option is fine too.

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.

2 participants