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

Add docs for Avatar, Icon; Reformat Badge docs #355

Merged
merged 6 commits into from
Jan 12, 2024

Conversation

martinxu9
Copy link
Contributor

@martinxu9 martinxu9 requested a review from Alek99 January 9, 2024 22:29

### High Contrast

We use the highContrast prop to increase color contrast of the fallback text with the background.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
We use the highContrast prop to increase color contrast of the fallback text with the background.
We use the `high_contrast` prop to increase color contrast of the fallback text with the background.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh .... thanks 🥲


### Radius

We use the radius prop to assign a specific radius value, ignoring the global theme.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
We use the radius prop to assign a specific radius value, ignoring the global theme.
We use the `radius` prop to assign a specific radius value, ignoring the global theme.

Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if we also want the "It can take values "none" | "small" | "medium" | "large" | "full"." bit as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, adding them in 22d1acd


### Fallback

We use the fallback prop to modify the rendered fallback text.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
We use the fallback prop to modify the rendered fallback text.
We use the `fallback` prop to modify the rendered fallback text.

Comment on lines 29 to 33
CalloutRoot.create(
CalloutIcon.create(icon(tag="check_circled", variant="solid", high_contrast=True, color="green")),
CalloutText.create("Below is a list of all available icons.", color="black", weight="bold"),
color="green",
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
CalloutRoot.create(
CalloutIcon.create(icon(tag="check_circled", variant="solid", high_contrast=True, color="green")),
CalloutText.create("Below is a list of all available icons.", color="black", weight="bold"),
color="green",
),
callout_root(
callout_icon(tag="check_circled", variant="solid", high_contrast=True, color="green")),
callout_text("Below is a list of all available icons.", color="black", weight="bold"),
color="green",
),

haven't tried if this actually works, but we should avoid explicitly calling .create throughout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The creates do work. Not sure why I thought the snake cases were not defined and I needed to do creates 🤦🏼 I was using the flex, icon, etc just fine


## Styling

The icons are based on the Radix primitive component, and it is unstyled. It does not accept common styling props such as `size` or `color_theme`. You can still use `width` and `height` together to control the size of the icon, and specify its `color`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The icons are based on the Radix primitive component, and it is unstyled. It does not accept common styling props such as `size` or `color_theme`. You can still use `width` and `height` together to control the size of the icon, and specify its `color`.
The icons are based on the Radix primitive component, and it is unstyled. **It does not accept common styling props such as `size` or `color_scheme`**. You can still use `width` and `height` together to control the size of the icon, and specify its `color`.

Comment on lines 78 to 81
icon(tag="magnifying_glass", color="indigo"),
icon(tag="magnifying_glass", color="cyan"),
icon(tag="magnifying_glass", color="orange"),
icon(tag="magnifying_glass", color="crimson"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

are these intended to be CSS colors? or radix themes colors?

i think at least one example should show color="var(--accent-10)" to link up with radix themes values.

Copy link
Contributor Author

@martinxu9 martinxu9 Jan 10, 2024

Choose a reason for hiding this comment

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

I want to say these are css colors? The component itself is supposedly primitive and does not take the common props. Just tried, the radix colors work, gonna add a couple examples there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in b550f41

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uploading image.png…

@martinxu9
Copy link
Contributor Author

revision uploaded! @masenf PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we break this up into sections like radix and include a link to radix icons at the top of the file as a source.

Copy link
Contributor

@Alek99 Alek99 Jan 11, 2024

Choose a reason for hiding this comment

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

Also maybe make them smaller and 4 or 5 width hard to scroll through and consume all of them

@martinxu9 martinxu9 merged commit 678f268 into radix-docs Jan 12, 2024
@picklelo picklelo deleted the martinxu9/data-display-p1 branch January 30, 2024 20:19
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.

None yet

3 participants