-
Notifications
You must be signed in to change notification settings - Fork 91
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
docs(fundamentals): fix example-row-detail positioning #1859
docs(fundamentals): fix example-row-detail positioning #1859
Conversation
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me from a dev perspective.
I have only left a small thing to address in a comment.
@CGuindon can give a final thumbs up from a design perspective.
@@ -108,7 +108,7 @@ | |||
</button> | |||
{% render 'example-row-detail.html', | |||
text: group.description, | |||
style: "margin-left: 50%;", | |||
classes: "mx-auto", | |||
customHeight: 16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Looking at the example-row-detail.html
partial I think that what we called customHeight
in this file should be called connectorHeight
instead.
Sidenote (out of scope)
I think we could find a better name for this partial. It took me a while to understand what Example Row Detail
even mean and I still have some doubt. Perhaps something likeannotation.html
or connected-annotation.html
could work at least based on its usage here. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: Looking at the
example-row-detail.html
partial I think that what we calledcustomHeight
in this file should be calledconnectorHeight
instead.
Ah, nice catch! I've resolved this in this commit.
Sidenote (out of scope) I think we could find a better name for this partial. It took me a while to understand what
Example Row Detail
even mean and I still have some doubt. Perhaps something likeannotation.html
orconnected-annotation.html
could work at least based on its usage here. Thoughts?
I agree. I think example-row-detail
is one of those placeholder names that just accidentally became semi-permanent. I think annotation.html
or example-annotation.html
makes the sense to me (I'd argue against connected-annotation.html
to keep the name tied to purpose as opposed to presentation).
I'll make a follow up PR for that to keep this PR focused. Thanks for the feedback.
Update: Here's the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, I have merged it now.
You might want to rebase this branch then. Hopefully there are not nasty conflicts to fix. 🤞
STACKS-657
This PR resolves positioning issues with the annotations used in color fundamentals docs page.
Before
After
How to test
npm run start
)