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 FormattedText component #72

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

alonmar
Copy link

@alonmar alonmar commented Dec 5, 2023

This is my PR proposal for #29

Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Merging #72 (bc87be2) into main (0f577cd) will not change coverage.
Report is 4 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #72   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          120       118    -2     
=========================================
- Hits           120       118    -2     

@alonmar alonmar changed the title add FormattedText component (#29) add FormattedText component Dec 5, 2023
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

we also need to change types types for Button and Paragraph to take list[FormattedText | str | Link] instead of just str.

None, serialization_alias='textFormat'
)
# TODO, use pydantic-extra-types Color?
text_color: str | None = pydantic.Field(None, serialization_alias='color')
Copy link
Member

Choose a reason for hiding this comment

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

you need to use Union, not | to support older python.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

looking good, thanks.

You'll need to rebase to main and fix JSON Schema equivalence tests which might be tricky, let me know if you need help.

c.Div(
components=[
c.Heading(text='FormattedText', level=2),
c.Markdown(text='`FormattedText` can be used to change the style of the text.'),
Copy link
Member

Choose a reason for hiding this comment

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

please add something like

this is particularly useful to avoid the overhead of loading the markdown renderer, or when you're including user generated content that you don't want to escape styling.

@@ -56,7 +56,7 @@ class Text(pydantic.BaseModel, extra='forbid'):


class Paragraph(pydantic.BaseModel, extra='forbid'):
text: str
text: '_t.List[_t.Union[str, FormattedText, Link]]'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
text: '_t.List[_t.Union[str, FormattedText, Link]]'
text: '_t.Union[str, _t.List[_t.Union[str, FormattedText, Link]]]'

you need to support just str as well.

Then you need to update Paragraph and Button to render this.

I guess Text should also be changed.

const style = {
backgroundColor,
color,
fontWeight: textFormat === 'bold' ? 'bold' : 'normal',
Copy link
Member

Choose a reason for hiding this comment

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

should probably be omitted, not normal if bold is not set.

backgroundColor,
color,
fontWeight: textFormat === 'bold' ? 'bold' : 'normal',
fontStyle: textFormat === 'italic' ? 'italic' : 'normal',
Copy link
Member

Choose a reason for hiding this comment

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

same.

@alonmar
Copy link
Author

alonmar commented Dec 13, 2023

looking good, thanks.

You'll need to rebase to main and fix JSON Schema equivalence tests which might be tricky, let me know if you need help.

Yes please, can you help me with that?

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.

3 participants