Skip to content
This repository has been archived by the owner on Jan 9, 2023. It is now read-only.

CB-11836 Allow setting of 'ForegroundText' property via config.xml #195

Closed
wants to merge 7 commits into from

Conversation

tsschaffert
Copy link
Contributor

We had a problem when trying to release an app in the Windows Store with BackgroundColor set to white because of the contrast between background and foreground color. With this PR, you can set the foreground color to 'dark' in these cases to pass store certification.

@codecov-io
Copy link

codecov-io commented Sep 8, 2016

Current coverage is 74.34% (diff: 100%)

Merging #195 into master will increase coverage by 0.09%

@@             master       #195   diff @@
==========================================
  Files            14         14          
  Lines          1942       1949     +7   
  Methods         362        366     +4   
  Messages          0          0          
  Branches        391        392     +1   
==========================================
+ Hits           1442       1449     +7   
  Misses          500        500          
  Partials          0          0          

Powered by Codecov. Last update 0855feb...55dadc8

@tsschaffert tsschaffert changed the title Allow setting of 'ForegroundText' property via config.xml CB-11836 Allow setting of 'ForegroundText' property via config.xml Sep 12, 2016
Copy link
Contributor

@daserge daserge left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution!
I've documented this in apache/cordova-docs#638

Copy link
Contributor

@daserge daserge left a comment

Choose a reason for hiding this comment

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

OK, actually there are some points which need to be updated:

  1. Windows 10 does not support ForegroundText (so we'll get a build failure if manifest contains ForegroundText),
  2. It is better to avoid color argument reassignment,
  3. It would be nice to cover the feature with some tests.

@daserge
Copy link
Contributor

daserge commented Sep 20, 2016

@tsschaffert, please let me know if you plan to address the comments.

@tsschaffert
Copy link
Contributor Author

@daserge I will look into it

@tsschaffert
Copy link
Contributor Author

  1. The issue with Windows 10 should be fixed now, I handled it similarly to the 'ToastCapable' property.
  2. The color argument is now passed to the manifest file unchecked, so invalid values return an error like 'App manifest validation failed.' when building the project. Alternatively, we could throw a TypeError for invalid values?
  3. I added only a very basic test, as there were no comparable tests for the visualElement methods and I don't know enough about the internals to write really meaningful tests

Copy link
Contributor

@daserge daserge left a comment

Choose a reason for hiding this comment

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

Thanks for the update!
I've left some comments.

@@ -390,11 +390,6 @@ AppxManifest.prototype.getVisualElements = function () {
},
setForegroundText: function (color) {
if (color) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of if (color) { visualElements.attrib.ForegroundText = color; } would it be better to use visualElements.attrib.ForegroundText = color || 'light' so that ForegroundText would be reset as default light if we set dark via preference for example and then remove the preference from config.xml entirely?

// Set to 'dark'
visualElementsWindows.setForegroundText(foregroundTextDark);
expect(visualElementsWindows.getForegroundText()).toEqual(foregroundTextDark);

Copy link
Contributor

@daserge daserge Sep 21, 2016

Choose a reason for hiding this comment

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

It also would be good to add a case for missing preference:

            // Simulate the absence of preference, should default to 'light'
            visualElementsWindows.setForegroundText(undefined);
            expect(visualElementsWindows.getForegroundText()).toEqual(foregroundTextLight);

@tsschaffert
Copy link
Contributor Author

Good points, I updated the changes accordingly.

Copy link
Contributor

@daserge daserge left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @tsschaffert!
I'm going to test and merge this.

@asfgit asfgit closed this in 10796b2 Sep 21, 2016
tsschaffert pushed a commit to pitlabs/cordova-windows that referenced this pull request Oct 27, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants