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 RichTextBuilder #1520

Merged
merged 8 commits into from
Jan 10, 2021
Merged

Add RichTextBuilder #1520

merged 8 commits into from
Jan 10, 2021

Conversation

maan2003
Copy link
Collaborator

@maan2003 maan2003 commented Jan 9, 2021

I have decided to move RichText into a separate module not sure if it is better to keep it inside storage

@maan2003
Copy link
Collaborator Author

maan2003 commented Jan 9, 2021

😅 I forgot cargo fmt and cargo test.

@maan2003
Copy link
Collaborator Author

maan2003 commented Jan 9, 2021

Maybe I should add methods like text_color, size on AttributesAdder. What do you think?

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Cool this looks good to me, I have a few little tweaks but overall happy to take this patch! :)

druid/src/text/rich_text.rs Outdated Show resolved Hide resolved
Comment on lines 98 to 101
/// # use druid::text::{RichTextBuilder, Attribute};
/// let mut rich_text = RichTextBuilder::new();
/// rich_text.push("Hello World").underline(true);
/// let rich_text = rich_text.build();
Copy link
Member

Choose a reason for hiding this comment

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

A slightly richer example:

Suggested change
/// # use druid::text::{RichTextBuilder, Attribute};
/// let mut rich_text = RichTextBuilder::new();
/// rich_text.push("Hello World").underline(true);
/// let rich_text = rich_text.build();
/// # use druid::text::{Attribute, RichTextBuilder};
/// # use druid::FontWeight;
/// let mut builder = RichTextBuilder::new();
/// builder.push("Hello ");
/// builder.push("World!").weight(FontWeight::Bold);
/// let rich_text = rich_text.build();

Self::default()
}

/// Add string to the end of text and `AttributesAdder` for the added string.
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
/// Add string to the end of text and `AttributesAdder` for the added string.
/// Append a `&str` to the end of the text.
///
/// This method returns a [`AttributesAddr`] that can be used to style the newly
/// added string slice.

Comment on lines 124 to 125
/// Get the `AttributesAdder` for the range.
pub fn range(&mut self, range: Range<usize>) -> AttributesAdder {
Copy link
Member

Choose a reason for hiding this comment

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

I'd give this a slightly more verbose name:

Suggested change
/// Get the `AttributesAdder` for the range.
pub fn range(&mut self, range: Range<usize>) -> AttributesAdder {
/// Get an [`AttributesAdder`] for the given range.
///
/// This can be used to modify styles for a given range after it has been added.
pub fn add_attributes_for_range(&mut self, range: Range<usize>) -> AttributesAdder {

@maan2003
Copy link
Collaborator Author

also changed add_attributes_for_range to take a impl RangeBounds

@maan2003
Copy link
Collaborator Author

I think squashing is a good idea 😅. Please add instructions to setup hooks on windows

@maan2003 maan2003 merged commit d8041c2 into linebender:master Jan 10, 2021
@maan2003 maan2003 deleted the rich-text-builder branch January 10, 2021 18:35
@cmyr
Copy link
Member

cmyr commented Jan 10, 2021

I think squashing is a good idea 😅. Please add instructions to setup hooks on windows

not sure what this is referring to :)

@maan2003
Copy link
Collaborator Author

maan2003 commented Jan 11, 2021

not sure what this is referring to :)

this doesn't work for windows.
https://github.com/linebender/druid/blob/master/CONTRIBUTING.md#before-opening-a-pr

@cmyr
Copy link
Member

cmyr commented Jan 14, 2021

@maan2003 hm I don't now much about this, but maybe search for "git hooks on windows" and see what's involved? would be happy to take a PR that updates those docs.

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