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

Dark mode editor support. #198

Merged
merged 13 commits into from Jan 24, 2019

Conversation

Lego6245
Copy link
Member

This is part RFC, part code review, for dark mode support in rooster js. For sample site implementation, please see forthcoming PR which will have this as a dependent.

Philosophy:
Editor, on create, will accept an additional parameter for dark mode support. If provided (either a boolean or a DarkModeOptions object), the editor will be in dark mode.

Dark mode is enabled by having two data propertie, ogsc and ogsb. ogsc represents the color for light mode, ogsb represents the background color for light mode. Conversion is currently one way.

Editor will expose two new APIs:

  • isDarkMode -> returns a boolean that will indicate if the editor is in dark mode or not
  • getDarkModeOptions -> returns the DarkModeOptions object.

Editor's API will change:

  • getContent will take an additional parameter. It will, by default, always return the "light" mode of content, unless passed a boolean to get the dark mode content, if it exists. Otherwise, it's a no-op if in light mode.
    There's a new helper method to convert from dark mode to light mode.

Editor Core will have two new properties, isDarkMode and darkModeOptions to facilitate the above. getDefaultFormat will rectify the dark mode options instead of the other defaults if dark mode is enabled.

DefaultFormat has been expanded to provide the ability to set default ogsc and ogsb properties. Dependent files (applyFormat) have been updated to allow for this as well.

setTextColor and setBackgroundColor both take in ogsc and ogsb parameters now.

Paste.ts now applies a transform to pasted content if dark mode is enabled. We provide our own default (strip color) but allow a user to define a behavior through onExternalContentTransform.

Soliciting feedback on the entire project, but I'll ask questions inline.

Copy link
Contributor

@Adjective-Object Adjective-Object left a comment

Choose a reason for hiding this comment

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

for setBackgroundColor, use of ogsb/c in code, misleading parameter names.

packages/roosterjs-editor-api/lib/format/setTextColor.ts Outdated Show resolved Hide resolved
packages/roosterjs-editor-api/lib/format/setTextColor.ts Outdated Show resolved Hide resolved
packages/roosterjs-editor-core/lib/editor/Editor.ts Outdated Show resolved Hide resolved
packages/roosterjs-editor-core/lib/editor/Editor.ts Outdated Show resolved Hide resolved
packages/roosterjs-editor-core/lib/undo/Undo.ts Outdated Show resolved Hide resolved
packages/roosterjs-editor-plugins/lib/Paste/Paste.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Adjective-Object Adjective-Object left a comment

Choose a reason for hiding this comment

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

Looks like you missed some changes in the refactor because of casting.

lgtm otherwise! 👍

packages/roosterjs-editor-api/lib/format/setTextColor.ts Outdated Show resolved Hide resolved
packages/roosterjs-editor-api/lib/format/setTextColor.ts Outdated Show resolved Hide resolved
packages/roosterjs-editor-types/lib/consts/DarkMode.ts Outdated Show resolved Hide resolved
packages/roosterjs-editor-dom/lib/utils/applyFormat.ts Outdated Show resolved Hide resolved
packages/roosterjs-editor-dom/lib/utils/applyFormat.ts Outdated Show resolved Hide resolved
@Lego6245 Lego6245 changed the base branch from master to roosterjs8-beta January 24, 2019 17:20
@Lego6245 Lego6245 closed this Jan 24, 2019
@Lego6245 Lego6245 changed the base branch from roosterjs8-beta to roosterjs-darkmode-alpha January 24, 2019 17:29
@Lego6245 Lego6245 reopened this Jan 24, 2019
if (defaultFormat.textColors) {
setTextColor(editor, defaultFormat.textColors);
} else {
setTextColor(editor, defaultFormat.textColor);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: setTextColor(editor, defaultFormat.textColors || defaultFormat.textColor);
(current way is also ok to me)

@adriantran
Copy link
Contributor

curious to know if this works with copy-paste scenarios

@Lego6245
Copy link
Member Author

Pasting content in will be supported (see the changes to Paste.ts)

Copying from editor into another surface currently isn't normalized. I'd do that outside this PR just because of its current size.

@Lego6245 Lego6245 merged commit d563b58 into microsoft:roosterjs-darkmode-alpha Jan 24, 2019
@Lego6245 Lego6245 deleted the darkmode-rooster branch January 24, 2019 23:46
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

5 participants