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

fixed: replace hard coded background color with themed color #2877

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

Conversation

daboe01
Copy link
Contributor

@daboe01 daboe01 commented Dec 10, 2019

previously, CPTextView used a card coded white background.
This PR makes the background color themeable

@cappbot cappbot added this to the Someday milestone Dec 10, 2019
@cappbot cappbot added the #new label Dec 10, 2019
@cappbot
Copy link

cappbot commented Dec 10, 2019

Milestone: Someday. Label: #new. What's next? A reviewer should examine this issue.

@kerusan
Copy link
Contributor

kerusan commented Dec 12, 2019

The setting of the background color need to be set whenever there is a state change. I think that maybe the drawRect: method is where it can be. Now it is set only in initWithFrame: and during drag operation.

@kerusan
Copy link
Contributor

kerusan commented Dec 12, 2019

Also there has to be a method called +(CPString)defaultThemeClass

@kerusan
Copy link
Contributor

kerusan commented Dec 13, 2019

As we in our project use a pattern color instead of color values, there has to be a style setting that sets transparency in the pattern, like

#if PLATFORM(DOM)
_DOMElement.style.cursor = "text";
_DOMElement.style.background = "transparent";

#endif

Then you can use both variants.

@kerusan
Copy link
Contributor

kerusan commented Dec 13, 2019

The background color can be be set as a background pattern and then the content inset has to be altered. So we must have that "content-inset" value too.

@didierkorthoudt
Copy link
Contributor

Found the caret position problem origin. Testing a fix.

@didierkorthoudt
Copy link
Contributor

Sadly not enough…

@daboe01
Copy link
Contributor Author

daboe01 commented Dec 15, 2019

@didierkorthoudt i reworked cared positioning in #2872. maybe @mrcarlberg can merge #2872 in?

@didierkorthoudt
Copy link
Contributor

@daboe01 Did you also adapt the initial caret position, like in my last commit ?

@daboe01
Copy link
Contributor Author

daboe01 commented Dec 15, 2019

@didierkorthoudt no, i missed that. no merge conflict ahead. thank you!

@mrcarlberg
Copy link
Member

Great work on this!

As this is a text view we also need to set a couple of theme states depending on what state the view is in. There is a need for this so the background can be different depending on the state. Examples of this could be CPThemeStateEditable, CPThemeStateHovered and CPThemeStateDisabled.

@mrcarlberg
Copy link
Member

Also, what about the suggestion from @kerusan to make the background style transparent?

@didierkorthoudt
Copy link
Contributor

@mrcarlberg I could add the various theme states, just be aware that this is a disruption from Cocoa (which is not a problem). Regarding the transparency of the background, we should then add a property ?

@kerusan
Copy link
Contributor

kerusan commented Dec 17, 2019

@didierkorthoudt Since theming is a "disruption" from Cocoa this could be done anyway.

@mrcarlberg
Copy link
Member

@didierkorthoudt Do you mean that a text view in Cocoa does have the same look when it is being edit or being disabled?

@kerusan
Copy link
Contributor

kerusan commented Dec 17, 2019

@didierkorthoudt If you compare with the CPTextField it has no property for transparence. I think the main reason for seeing the dom property "background" to "transparent" was to allow setting of color patterns. There it makes sense.

@didierkorthoudt
Copy link
Contributor

@mrcarlberg For what I can see on a test app, yes, there's no visual clue that a text field is editable or not (there's no disabled state !)... Sometimes Cocoa is not very consistent. There's even no focus ring !..

@didierkorthoudt
Copy link
Contributor

@kerusan OK, I'll add this ;-)

@didierkorthoudt
Copy link
Contributor

@kerusan Well, maybe that is could be done with a CSS Color (the one created for Aristo3 and normally already present in 1.0) ? What do you think ?

@mrcarlberg
Copy link
Member

@didierkorthoudt Ok, but we have these states on CPTextField. I think we should add those on CPTextView too. So we are compliant with our self 😄

@didierkorthoudt
Copy link
Contributor

@mrcarlberg OK, that's a perfect criteria !.. 😄 I'll work on this.

@kerusan
Copy link
Contributor

kerusan commented Dec 17, 2019

@didierkorthoudt I'm not familiar with the CSS solution I just tried to copy the CPTextField solution and it works ;-)

@daboe01
Copy link
Contributor Author

daboe01 commented Jan 14, 2020

-#new
+TextView
+Theme

@cappbot
Copy link

cappbot commented Jan 14, 2020

Milestone: Someday. Labels: TextView, Theme. What's next? A reviewer should examine this issue.

@cappbot
Copy link

cappbot commented Feb 21, 2020

Milestone: Someday. Labels: #needs-improvement, TextView, Theme. What's next? The code for this issue has problems with formatting or fails a capp_lint check, has bugs, or has non-optimal logic or algorithms. It should be improved upon.

@daboe01
Copy link
Contributor Author

daboe01 commented Apr 28, 2021

@mrcarlberg can you please elaborate what needs to be done to get this ready to commit?

@daboe01
Copy link
Contributor Author

daboe01 commented Mar 20, 2022

@mrcarlberg looks good to me. what needs to be improved?

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.

None yet

5 participants