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.

Sign up for GitHub

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

Jump to bottom

PBL-21894 Add color support for window layout editor #222

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

PBL-21894 Add color support for window layout editor #222

wants to merge 10 commits into from

Conversation

Spacerat
Copy link
Contributor

@Spacerat Spacerat commented Oct 2, 2015

Preview
This PR adds full support for colours to the Interface Builder, with fallbacks supported for B/W watches.

@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-222 October 2, 2015 19:15 Inactive
@Katharine
Copy link
Contributor

I question your sense of colour.

Katharine
Katharine reviewed Oct 2, 2015
View reviewed changes
ide/static/ide/js/ib/layers/actionbarlayer.js
@@ -48,7 +48,8 @@
this._node.css({
'background-color': this._backgroundColour.getValue().css
});
var invertIcons = (this._backgroundColour.getValue() != IB.ColourWhite);
var mono_bg_colour = this._backgroundColour.getValue(IB.ColourModes.Monochrome);
Copy link
Contributor

Choose a reason for hiding this comment

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

Camel case in the UI editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've changed it everywhere

@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-222 October 2, 2015 20:19 Inactive
@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-222 October 2, 2015 21:12 Inactive
@Katharine
Copy link
Contributor

This has merge conflicts.

@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-222 October 22, 2015 00:49 Inactive
@Spacerat
Copy link
Contributor Author

Conflicts fixed!

@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-222 December 16, 2015 21:08 Inactive
Katharine
Katharine reviewed Jan 11, 2016
View reviewed changes
ide/static/ide/js/colour_picker.js
@@ -0,0 +1,143 @@
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this inside the anonymous function (as the first statement).

@Katharine
Copy link
Contributor

The appearance of ActionBarLayer should be updated to match its 3.x appearance (a 30-pixel-wide bar down the side; you'll have to check icon positioning).

@Spacerat
Copy link
Contributor Author

Would you want to that to change depending on whether the user is working with 2.x or 3.x?

@Katharine
Copy link
Contributor

Yes.

@Spacerat
Copy link
Contributor Author

Hmm, looking at it, it seems I never actually added anything to disable the colour features when making a 2.x project...

@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-222 January 12, 2016 00:13 Inactive
@Spacerat
Copy link
Contributor Author

OK, I've fixed the code issues, the ActionBarLayer display, and made the colour options SDK 3 only

@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-222 January 12, 2016 00:17 Inactive
@Katharine
Copy link
Contributor

I wonder if the various other globals you've added (IB.Settings, IB.ColourEnabled) should also be settings, colourEnabled, etc.?

@Spacerat
Copy link
Contributor Author

IB.Settings is a submodule like ResourceManager/etc. ColourEnabled I wasn't sure about, since it's effectively a constant for the lifetime of the page.

@Katharine
Copy link
Contributor

if(constant) never makes sense; lets change that one.

@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-222 January 12, 2016 01:19 Inactive
@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-222 January 12, 2016 01:19 Inactive
@Spacerat
Copy link
Contributor Author

Right, PBIs will work in the ActionBarLayer now, and I renamed ColourEnabled to colourEnabled.

Katharine
Katharine reviewed Jan 12, 2016
View reviewed changes
ide/static/ide/js/ib/utils.js
GColorClear: new IB.Colour('GColorClear', 'rgba(0, 0, 0, 0)', gettext('Transparent'))
};
_.each(IB.FullColourDescriptionMap, function(colour) {
var GColor = _.findWhere(colour.literals, {'id': 'define'}).value;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably just be called gcolour.

@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-222 January 12, 2016 01:37 Inactive
@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-222 January 13, 2016 20:20 Inactive
@pebble-heroku pebble-heroku temporarily deployed to cloudpebble-sp-pr-222 January 20, 2016 23:37 Inactive
@Spacerat Spacerat temporarily deployed to cloudpebble-sp-pr-222 May 9, 2016 18:33 Inactive
@Spacerat Spacerat temporarily deployed to cloudpebble-sp-pr-222 June 7, 2016 17:50 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Spacerat @Katharine @pebble-heroku