Navigation Menu

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

WS10 Add support for new themes format #273

Merged
merged 21 commits into from Dec 8, 2022
Merged

Conversation

obany
Copy link
Contributor

@obany obany commented Dec 2, 2022

WIP - waiting for WS bugfixes

@obany obany changed the title Add support for WS10 themes WS10 Add support for new themes format Dec 2, 2022
Copy link
Contributor

@johnman johnman left a comment

Choose a reason for hiding this comment

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

It seems like you've had to do more work than you should need to (I wonder how dock icons would react with a light/dark switch) as they can't be updated. Can you take me through it tomorrow?

export async function updateBrowserWindowButtonsColorScheme(
browserWindow: BrowserWindowModule
): Promise<void> {
const options = await browserWindow.openfinWindow.getOptions();
Copy link
Contributor

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.

Copy link
Contributor Author

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 path

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor Author

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

options: OpenFin.PlatformWindowCreationOptions,
identity?: OpenFin.Identity
): Promise<OpenFin.Window> {
const window = await super.createWindow(options, identity);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this get triggered by classic windows?

export async function updateBrowserWindowButtonsColorScheme(
browserWindow: BrowserWindowModule
): Promise<void> {
const options = await browserWindow.openfinWindow.getOptions();
Copy link
Contributor

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?


public async setSelectedScheme(schemeType: ColorSchemeOptionType) {
// The color scheme has been updated, so update the theme
await setCurrentColorSchemeMode(schemeType);
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we could certainly do it that way as well

Copy link
Contributor

Choose a reason for hiding this comment

The 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).

export async function getWindowState(): Promise<"normal" | "minimized" | "none"> {
if (registrationInfo) {
try {
const win = fin.Window.wrapSync({ name: "openfin-dock", uuid: "openfin-browser" });
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be doing this. It was done in the past when there was no sdk but we shouldn't be dependant on uuid (it can change) and dock name. This should be a feature request for the api to expose state (e.g. isVisible)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@obany obany merged commit 20ce57d into workspace/vnext Dec 8, 2022
@obany obany deleted the dev/martyn/ws10-themes branch December 8, 2022 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants