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

feature/Support for multiple styles in same text #447

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Fernando-hub527
Copy link
Contributor

Description
This pr allows you to merge different styles in the same text, to do this merge it is necessary that when creating the component, a list of subTexts is sent, each subText with its own style.

Related Issue
#189

Checklist

check with "x", ONLY IF APPLIED to your change

  • All methods associated with structs has func (<first letter of struct> *struct) method() {} name style.
  • Wrote unit tests for new/changed features.
  • Followed the unit test when,should naming pattern.
  • All mocks created with m := mocks.NewConstructor(t).
  • All mocks using m.EXPECT().MethodName() method to mock methods.
  • Updated docs/doc.go and docs/*
  • Updated example_test.go.
  • Updated README.md
  • New public methods/structs/interfaces has comments upside them explaining they responsibilities
  • Executed make dod with none issues pointed out by golangci-lint

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 18.40491% with 133 lines in your changes missing coverage. Please review.

Project coverage is 83.38%. Comparing base (8cc69ea) to head (eebd7e5).

Files Patch % Lines
internal/providers/gofpdf/text.go 0.00% 96 Missing ⚠️
pkg/props/text.go 0.00% 30 Missing ⚠️
pkg/core/entity/subText.go 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #447      +/-   ##
==========================================
- Coverage   85.71%   83.38%   -2.33%     
==========================================
  Files          61       62       +1     
  Lines        2155     2243      +88     
==========================================
+ Hits         1847     1870      +23     
- Misses        281      346      +65     
  Partials       27       27              
Flag Coverage Δ
unittests 83.38% <18.41%> (-2.33%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@johnfercher johnfercher left a comment

Choose a reason for hiding this comment

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

I will do a better review tomorrow, I have to download you fork and see the implementation better in text component.

Comment on lines +94 to +96
subText1 := entity.NewSubText("This is a text", props.SubText{Color: &props.BlueColor})
subText2 := entity.NewSubText(" with multiple", props.SubText{Size: 7})
subText3 := entity.NewSubText(" styles", props.SubText{Color: &props.RedColor})
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe NewSubText should be in text package also as NewCustomText.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to do it this way, but the provider depends on the subText structure and if I declare the subtext in the Text package, it generates a cyclical dependency.
I think this would be possible if we remove the dependency that the provider has on subtext, but I don't know how to do this in a nice way

subText2 := entity.NewSubText(" with multiple", props.SubText{Size: 7})
subText3 := entity.NewSubText(" styles", props.SubText{Color: &props.RedColor})

customText := col.New(12).Add(text.NewCustomText([]*entity.SubText{subText1, subText2, subText3}))
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we could change the Add([]text) to Add(text...). With a vargar we can still pass an array with as NewCustomText(arr...) and we can pass each one as this NewCustomText(subText1, subText2, subText3) this is a more extensible and idiomatic way to work with collections in go.

Copy link
Contributor Author

@Fernando-hub527 Fernando-hub527 Jun 27, 2024

Choose a reason for hiding this comment

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

I agree, this way is better. The only issue is that NewCustomText also needs to receive text.Props and I'm already using vararg in this parameter, but I agree that using vargar in the subText parameter is better. The function call would look like this: NewCustomText(props.Text{}, subText1, subText2, subText3), do you think it looks good this way?

@@ -48,6 +48,10 @@ func (g *provider) AddText(text string, cell *entity.Cell, prop *props.Text) {
g.text.Add(text, cell, prop)
}

func (g *provider) AddCustomText(subs []*entity.SubText, cell *entity.Cell, textPs *props.Text) {
Copy link
Owner

Choose a reason for hiding this comment

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

Same comment here. vararg instea of array.

// This method is responsible for allowing the union of a set of subtexts in the same text
func (s *text) AddCustomText(subs []*entity.SubText, cell *entity.Cell, textPs *props.Text) {
originalColor := s.font.GetColor()
defer s.font.SetColor(originalColor)
Copy link
Owner

Choose a reason for hiding this comment

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

Awesome use of defer...I'm thinking that I should do this more in some places here too. xD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice !! In the next issues I resolve I will keep an eye out for places where we can use it

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