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

feat: add option to change text color #689

Merged
merged 3 commits into from
Oct 10, 2022

Conversation

Wolfy7
Copy link
Contributor

@Wolfy7 Wolfy7 commented Oct 3, 2022

Please check if the PR fulfills these requirements:

  • The commit message follows our guidelines.
  • For bug fixes and features:
    • You tested the changes.

Related issue (if applicable): #623

What kind of change does this PR introduce?
Adds new Setting Text Color to the SettingsPopup to change the color of all (Label and RichTextLabel ) text in the app.
Color setting is also stored to the Profile.

New feature or change

image

image

image
image

Other information
Maybe its not the best idea to change Label and RichTextLabel or probably there are Control Nodes who should change color.

@NathanLovato
Copy link
Contributor

Thanks for your contribution!

Instead of giving a color picker, I would rather provide a text contrast slider or toggle button and map it to carefully hand-picked colors. This is to ensure that the user cannot, for example, pick a color that makes the app unusable. But also, they shouldn't be able to pick red or the purple color used in the background (which would make the text invisible).

On top of that, highlighted keywords in the text use different colors than the main text. They may need adjustments too, and having predefined colors depending on the desired contrast level would allow us to provide a cohesive color palette.

Writing this, I think that a "Lower contrast" toggle button would be a good option to try. Having a single low-contrast option that changes only the text color for now (later it could also tweak other colors).

Note that it needs to affect the lesson text, but also the TextEdit palette (code editor in the practice UI).

@Wolfy7
Copy link
Contributor Author

Wolfy7 commented Oct 4, 2022

Yes, you right.
During the implementation i also thought that this will be error prone.

I also thought on something like diffrent color themes that could be chosse or something, but probably this is then too much to maintain.

A "Lower contrast" toggle button option sounds like a good approach, at least for the first step.
Do you have some "lower contrast" color already in mind? And probably you now which Control Notes (Label , RichTextLabel) should affected?

Thansk for helping.

@NathanLovato
Copy link
Contributor

I also thought on something like diffrent color themes that could be chosse or something, but probably this is then too much to maintain.

Yes, that would be a neat feature, but it's extra work and maintenance for something low-priority right now.

Do you have some "lower contrast" color already in mind? And probably you now which Control Notes (Label , RichTextLabel) should affected?

Would you be comfortable writing the code and implementing the feature with placeholder colors? We could then jump in and tweak the color palette on top of your pull request.

Regarding the nodes, this will first need to affect Label and RichTextLabel.

Then there are various TextEdit nodes. Those are trickier. To understand why to see CodeEditorEnhancer.gd: The text edit class doesn't have a great API. Whenever we register a new keyword, we need to hard code the color of this keyword instead of using the theme. Also, it has to be done separately on every text edit node, and this can't be undone: to change the colors, you must delete the text edits and instantiate new ones. So to add support for changing the contrast of the highlighted keywords, I think that we will need more refactoring.

So in this is fine by you, I would recommend focusing on just the label and rich text label nodes to start with. And just the normal text color. Once this is done, we can consider the text edit nodes and keyword highlighting in lessons.

@Wolfy7
Copy link
Contributor Author

Wolfy7 commented Oct 4, 2022

Yes, i could write the code with placeholders for the color(s).

But jsut to understand everything correctly, which approach we want now to start with:

  • A "CheckButton" to just toogle between "normal" and "Lower contrast"?
    • So just one placeholder color is needed.
  • Something like a slider or table to to select the "Lower contrast" color from some color suggestions

At the moment i already handle Label and RichTextLabel, so i then only have to replace the ColorPicker by the chosen approach.

After that i could check CodeEditorEnhancer.gd to get an overview how maybe TextEdit nodes could be handeld.

@NathanLovato
Copy link
Contributor

A "CheckButton" to just toogle between "normal" and "Lower contrast"?

Yes! I think it's best to keep things as simple as possible by default and only add more options as people really need them.

@NathanLovato
Copy link
Contributor

After that i could check CodeEditorEnhancer.gd to get an overview how maybe TextEdit nodes could be handled.

For this we will need to delete the existing text edit nodes and replace them or just rebuild the lesson UI or the practice UI and restore the student´s work state, whichever works best. I unfortunately don't think the API gives us many more options.

@Wolfy7
Copy link
Contributor Author

Wolfy7 commented Oct 4, 2022

With commit ed582ba

Added a CheckButton to the SettingsPopup to enable/disable Lower contrast color for Label and RichTextLabel.
Defined a Placeholder for "lower contrast color" and "Defaullt color"

@Wolfy7
Copy link
Contributor Author

Wolfy7 commented Oct 10, 2022

Hi Nathan.

Are these changes are okay for now? Will you change the "Default color"?

@NathanLovato
Copy link
Contributor

Sure, thanks for the bump, I'm adding this to my to do list. The changes look good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants