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

Stack reimplentation #93

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Stack reimplentation #93

wants to merge 3 commits into from

Conversation

siddharthkp
Copy link
Owner

@siddharthkp siddharthkp commented May 21, 2020

Re-implemented the Stack one more time based on the working draft spec. Wrote down my notes here - http://sid.st/unpolished/flex-gap-polyfill

update: ugh white space and line height will create a bunch of problems to fix

@vercel
Copy link

vercel bot commented May 21, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sid/react-ui/azpmt9wo9
✅ Preview: https://react-ui-git-stack-reimplentation.sid.now.sh

@siddharthkp siddharthkp marked this pull request as ready for review May 21, 2020 21:15
@siddharthkp siddharthkp requested a review from rubenmoya May 21, 2020 21:15
@siddharthkp siddharthkp marked this pull request as draft May 21, 2020 21:19
@siddharthkp siddharthkp removed the request for review from rubenmoya May 21, 2020 21:19
@rubenmoya
Copy link
Collaborator

The UI tests are failing, could it be because we're adding a stack manually to the examples but the Preview element is also a stack?

I like the children approach, the only improvement I can think of is checking if there is only one children and returning it instead of wrapping it in a div, although if you're using a Stack the chances of that are pretty low.

Why do we prefer using a margin-top instead of a padding-top?

Also, with this approach there is another problem, the divs or spans that we create will be aligned, but not the items inside them (not sure how to explain this 😅), if you check out the gif, the image inside the first div we add is not centered:

stack

What do you mean about line height and white space?

@siddharthkp
Copy link
Owner Author

The UI tests are failing, could it be because we're adding a stack manually to the examples but the Preview element is also a stack?

Yeah, I was too clever with the Preview component, will simplify it to get more explicit tests

Why do we prefer using a margin-top instead of a padding-top?

No reason :) Would be interesting to know why others do it

Also, with this approach there is another problem, the divs or spans that we create will be aligned, but not the items inside them (not sure how to explain this 😅), if you check out the gif, the image inside the first div we add is not centered:

I know what you mean. I really struggled with this - the container has it's own line height which breaks the vertical alignment in most of the tests :(

@siddharthkp siddharthkp changed the base branch from master to main June 27, 2020 08:53
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