Added Importance field on Label to color the text

[blaize]

Description:

I'm making a cross platform GUI app that manage acounting data and I wanted to color texts for negative values (red) and positive values (green). Also some warning messages if the client havn't paid should be printed in orange.
I can use canvas.Text but then I lost the TextStyle{} and Wrapping feature.
Also, using label and text in the same row of a List are not exactly aligned.

So I made this little patch based on the logic of ButtonImportance to add an Importance to a label. The Importance color the message on the label.

I'm not sure about names, and if it need to add an importance that print in Primary color and Secondary color.
But I think a lot of use case needs success, warning and error messages in the app.
Coloring text make the message more clear and the interface more intuitive.

Fixes #(issue)

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[andydotxyz]

I hadn't thought about this, could be cool.
I wonder if instead of adding text importance as a new API maybe we should either re-use the button ones or move to a less widget-specific naming that can be used elsewhere?

Or just export the ColorName directly on Label - not sure about that either...
Maybe others have thoughts on the naming - always good to get a few thoughts.

[coveralls]

coverage: 66.199% (+0.05%) from 66.149% when pulling 06bde3c on blaize:label-with-importance into e050d79 on fyne-io:develop.

[blaize]

I wonder if instead of adding text importance as a new API maybe we should either re-use the button ones or move to a less widget-specific naming that can be used elsewhere?

That what I was thinking at first, but I didn't want to use ButtonImportance for something called Label, and I also didn't want to move too much code for my first attempt so I made this. But yes, ideally an "importance" type that work for multiple widget would be the best.

[Jacalz]

I think we should deprecate widget.ButtonImportance and create a widget.Importance (or similar) and use that instead. We might even make the types entirely compatible by doing something like type Importance = ButtonImportance. That way, we can update the buttons to use the new type without it being a breaking change.

[blaize]

I commit a patch to use widget.Importance instead of widget.ButtonImportance.
Also updated label and other widget that used ButtonImportance.

[Jacalz]

Thanks. I added a few notes inline. Also, would you mind adding some tests? You can probably have a look at the existing importance tests for Button.

[blaize]

I resolved the comments you made and added some tests.

[Jacalz]

Thanks. I'll try to review this in a few days at most.

FYI: We usually prefer to let the reviewer mark their own comments as resolved (the one that request something decides on when it is done). It's no biggie this time but you know to the next time :)

[blaize]

Oh ok sorry I didn't know. I will not touch next time ;)

Thanks.

[blaize]

I added initialization on Medium importance on the label. It's not necessary because Medium = 0 but I think it's more readable.
Maybe I can do the same for button if you want, or you can discard the last commit.

Also, I copied the test and I notified this strange call in many test :

test.NewApp()
defer test.NewApp()

I don't understand why this is called second time at the end of the test. Can you explain me ?

[Jacalz]

Thanks. Just left a few minor notes inline.

I added initialization on Medium importance on the label. It's not necessary because Medium = 0 but I think it's more readable.

We generally try to not set default values. You can create a label or button without using the constructor as well. In my opinion it makes anyone reading the code think that it isn't the default so they set it when they don't need to.

[Jacalz]

I don't think you need any image for this if you just want to make sure that it compiles (please also start comments with a space and then a capital letter):

Suggested change

	test.NewApp()
	defer test.NewApp()
	test.ApplyTheme(t, theme.LightTheme())
	//test backward compatibility of widget.Importance
	var imp widget.ButtonImportance = widget.HighImportance
	btn := widget.NewButton("test", func() {})
	btn.Importance = imp
	btn.Refresh()
	w := test.NewWindow(btn)
	defer w.Close()
	test.AssertImageMatches(t, "button/compat_importance.png", w.Canvas().Capture())
	// Test backward compatibility of widget.Importance
	var imp widget.ButtonImportance = widget.HighImportance
	btn := widget.NewButton("test", func() {})
	btn.Importance = imp

Remember to delete the corresponding image as well.

[Jacalz]

This should not be needed. MediumImportance is equal to zero and thus is the default.

[Jacalz]

FYI: Please fill out all of the checkboxes in the PR description.

[Jacalz]

FYI: Please fill out all of the checkboxes in the PR description.

Sorry for the confusion. I meant all of those that you have done. You can delete those under "Where applicable:" that don't fit in with your current PR.

[Jacalz]

The code looks good to me. I'll do a proper review and test locally when I have some time over.

[Jacalz]

This looks good to me. Thanks. Well done

[andydotxyz]

Great thanks for adding this