Fixed: selected rows in CPTableViews are barely readable in unfocussed state #2009

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
8 participants
Contributor

daboe01 commented Oct 27, 2013

Introducing a CPThemeStateSelectedDataViewFocussed theme state, associating black text color with unfocussed and white text color in focussed state as is the same in Cocoa.

Fixes #2001

Fixed: selected rows in CPTableViews are barely readable in unfocusse…
…d state

Introducing a CPThemeStateSelectedDataViewFocussed theme state, associating black text color with unfocussed and white text color in focussed state as is the same in Cocoa.

Fixes #2001

cappbot commented Oct 27, 2013

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

timeimp commented Oct 27, 2013

CPThemeStateSelectedDataViewFocu s ed

:P

AppKit/CPTableView.j
- [view performSelector:selectors[info.selectorIndex] withObject:CPThemeStateSelectedDataView];
- }
- }
+ { var dataViewsInTableColumn = _dataViewsForTableColumns[identifier];
@primalmotion

primalmotion Oct 29, 2013

Owner

Style is wrong here

AppKit/CPTableView.j
+ var count = deselectColumns.length;
+ while (count--)
+ {
+ var columnIndex = deselectColumns[count],
Owner

primalmotion commented Oct 29, 2013

It seems your editor screwed up the style. I'm not sure if it's tabs or something, but please don't use tabs, only 4 spaces "tabs"

Thanks

AppKit/CPTableView.j
for (var identifier in _dataViewsForTableColumns)
{
var dataViewsInTableColumn = _dataViewsForTableColumns[identifier];
-
- for (var i = 0; i < selectInfo.length; ++i)
+ var count= deselectRows.length;
@primalmotion

primalmotion Nov 6, 2013

Owner

missing space here :)

Owner

primalmotion commented Nov 19, 2013

I'm not a big fan of the naming of CPThemeStateSelectedDataViewFocused. why do we limit this theme to the selected dataViews data view? While I'm all for that concept, wouldn't it be a more elegant fix to have a new theme state named like CPThemeStateFirstResponder, that we should apply everywhere in CPResponders? This would also reduce the number of different very specific theme states like CPThemeStateEditing.

Just random thoughts here...

Contributor

aparajita commented Nov 19, 2013

+1 on CPThemeStateFirstResponder

Owner

aljungberg commented Nov 19, 2013

Yeah +1 for a more generic and reusable theme state such as CPThemeStateFirstResponder, like we discussed the other day. The fewer theme states we have the easier it is for people creating themes and programmers alike.

Member

Dogild commented Nov 19, 2013

+1 Definitely

Contributor

ahankinson commented May 5, 2014

-#new
+AppKit
+Theme
+#ready-to-commit

cappbot commented May 5, 2014

Milestone: Someday. Labels: #ready-to-commit, AppKit, Theme. What's next? The changes for this issue are ready to be committed by a member of the core team.

Owner

aljungberg commented May 9, 2014

Ok so now the state is called CPThemeStateFirstResponder - but can't we now go ahead and make it more general rather than this very special case in CPTableView?

When a view becomes the first responder it could add the first responder theme state to itself and all children, and then remove it again if it loses that state. If we do this it becomes easy to theme "first responder" looks like focus rings and special highlight colours without additional work in any of the controls.

It would obviously have a performance impact on first responder handovers to toggle all those theme states, but I imagine it'd be much less than a human would notice when tabbing around the UI.

Contributor

daboe01 commented May 10, 2014

this suggestion sounds reasonable to me. i will close this request.

@daboe01 daboe01 closed this May 10, 2014

aljungberg added a commit that referenced this pull request May 12, 2014

Fixed: illegible white on grey text in table view.
If a table view row is selected its text will turn white to offset against the blue selection colour. However, when the table is not the first responder, or the window is in is not the key window, the light grey highlight colour is used instead, but the text remains white.

This fix ensures the text becomes black in this case by utilising the new first responder and key window theme states.

Fixes #2009, fixes #2001.

aljungberg added a commit that referenced this pull request May 12, 2014

Fixed: table view selection text colour in Aristo 1.
This is the same fix as in a4702d4 but applied to Aristo 1.

Refs #2009, refs #2001.
Owner

aljungberg commented May 12, 2014

Now fixed with alternative code as discussed.

+#fixed
assignee=aljungberg
milestone=0.9.8

cappbot commented May 12, 2014

Milestone: 0.9.8. Labels: #fixed, AppKit, Theme. What's next? This issue is considered successfully resolved.

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