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
WS10 Add support for new themes format #273
Changes from 15 commits
bc1d080
3296b93
65c5547
36b12a7
56fce32
89502ad
c355d4c
be36a53
b466cc2
f533eb1
91b1562
4964124
67ae18e
36e24a5
d5d0bf8
a6cc28a
df49bd7
ff7cd01
2a12187
1ca366f
3956294
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import { | ||
ColorSchemeOptionType, | ||
CreateSavedPageRequest, | ||
CreateSavedWorkspaceRequest, | ||
getCurrentSync, | ||
|
@@ -11,11 +12,14 @@ import { | |
Workspace, | ||
WorkspacePlatformOverrideCallback | ||
} from "@openfin/workspace-platform"; | ||
import { updateBrowserWindowButtonsColorScheme, updateButtonColorScheme } from "../buttons"; | ||
import * as endpointProvider from "../endpoint"; | ||
import { fireLifecycleEvent } from "../lifecycle"; | ||
import { createLogger } from "../logger-provider"; | ||
import { getGlobalMenu, getPageMenu, getViewMenu } from "../menu"; | ||
import { applyClientSnapshot, decorateSnapshot } from "../snapshot-source"; | ||
import { setCurrentColorSchemeMode } from "../themes"; | ||
import { updateDockColorScheme } from "../workspace/dock"; | ||
import { deletePageBounds, savePageBounds } from "./browser"; | ||
import { closedown as closedownPlatform } from "./platform"; | ||
|
||
|
@@ -292,6 +296,36 @@ export const overrideCallback: WorkspacePlatformOverrideCallback = async (Worksp | |
return super.quit(payload, callerIdentity); | ||
} | ||
} | ||
|
||
public async createWindow( | ||
options: OpenFin.PlatformWindowCreationOptions, | ||
identity?: OpenFin.Identity | ||
): Promise<OpenFin.Window> { | ||
const window = await super.createWindow(options, identity); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this get triggered by classic windows? |
||
|
||
try { | ||
const platform = getCurrentSync(); | ||
const browserWindow = platform.Browser.wrapSync(window.identity); | ||
await updateBrowserWindowButtonsColorScheme(browserWindow); | ||
} catch { | ||
// Probably not a browser window | ||
} | ||
|
||
return window; | ||
} | ||
|
||
public async setSelectedScheme(schemeType: ColorSchemeOptionType) { | ||
// The color scheme has been updated, so update the theme | ||
await setCurrentColorSchemeMode(schemeType); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rather than adding more and more logic here over time...could we not update the theme.ts via setCurrentColorScheme and that triggers a lifecycle event of theme changed? That way dock, and browser could register an action against that lifecycle event and if a platform dev wanted to hook into they could define a module and register it against the event as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes we could certainly do it that way as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that way if we have a custom button it just cares about telling theme.ts to switch and the lifecycle event will trigger everything else (rather than all the logic being in the override). |
||
|
||
// Also update toolbar buttons | ||
await updateButtonColorScheme(); | ||
|
||
// And relaunch the dock as we currently can't dynamically update buttons | ||
await updateDockColorScheme(); | ||
|
||
return super.setSelectedScheme(schemeType); | ||
} | ||
} | ||
return new Override(); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it should be part of the button definition with v10 sdk. i.e. provide two sets of icons and the workspace code does this work. We can catch up tomorrow on whether we should propose it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the syntax in the definition, you dont need to declare a separate themes section, just use
{theme}
in the pathThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does that break backwards compatibility? Do we need to make a note of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add to changelog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this might break sales studio as well so we might need v10 logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see anything themed icon paths in Sales Studio