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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix theme path issue #3048

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@AWolf81
Copy link
Contributor

commented May 30, 2019

Description

  • Changed path usage with-out ../
  • Change development check on process.env - not perfect but that way we're getting production stuff if NODE_ENV is not defined. I think we can fix / improve that once we improved the build process.

Interface theming had a path issue as mentioned in the issue.
Editor theming should be fixed as well.

Issue fixed

#3018, #3042

Type of changes

  • 馃敇 Bug fix (Change that fixed an issue)
  • 鈿笍 Breaking change (Change that can cause existing functionality to change)
  • 鈿笍 Improvement (Change that improves the code. Maybe performance or development improvement)
  • 鈿笍 Feature (Change that adds new functionality)
  • 鈿笍 Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • 馃敇 My code follows the project code style
  • 鈿笍 I have written test for my code and it has been tested
  • 馃敇 All existing tests have been passed
  • 馃敇 I have attached a screenshot/video to visualize my change if possible

Screenshot

grafik

Error message
grafik

Questions / notes

  • Please double check the editor theming.
  • Error message (see screenshot) - why do I get it?

Tested

  • LinuxMint 19.1 (4.15.0-50 Linux) - Dev server & binary - theme switching OK
  • Windows 10 64bit - Dev server & binary - theme switching OK
  • MacOs High Sierra (10.13.6) - Theme switching OK - tested by incredibleweirdo
@AWolf81

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

Seems like there is an issue on Linux. Tested with Ubuntu 18 and css not found - I need to do more debugging here.

Update
Fixed theming for Linux binary. I couldn't test the dev server in Ubuntu because it wasn't starting - no error it was just the stopped loading spinner. Will check this later.

AWolf81 added some commits May 30, 2019

@incredibleweirdo

This comment has been minimized.

Copy link

commented May 30, 2019

Hello,
I have tested on Windows 7 and macOS High Sierra (10.13.6), and can confirm themes now work as intended.
As I was guessing this was related to #2968 based on the fix discussed in #3018 making export usable, I tested that as well and found it is now solved in Windows 7 with this change; but still broken in macOS. I can capture console output and screenshots if that is helpful.

Edited to add:
I believe this breaks export on macOS.
If I yarn run dev from master or v0.11.16, I'm able to export on macOS without issue. With the branch from this PR, export on macOS does not work.
However, themes do work as expected, but themes also work on macOS before this change in my testing.

On Windows 7, master and v0.11.16 have the bug with themes not working, AND have export not working. The branch from this PR have themes working and export working.

I have a Linux machine at home I can test with tomorrow.

return `${appPath}/node_modules/codemirror/theme/elegant.css`
}
return theme
? (win ? theme.path : `../${theme.path}`)

This comment has been minimized.

Copy link
@incredibleweirdo

incredibleweirdo May 30, 2019

I think this is what it needs to be for macOS:

Suggested change
? (win ? theme.path : `../${theme.path}`)
? (win ? theme.path : `${appPath}/${theme.path}`)

This comment has been minimized.

Copy link
@AWolf81

AWolf81 May 31, 2019

Author Contributor

OK, changed this & tested it with Ubuntu binary. Theming (Editor, in preview code block & app theme) is working.

@amedora amedora referenced this pull request May 31, 2019

Open

[WIP] Fix #2776 #3051

@@ -141,7 +140,7 @@ function get () {
const theme = consts.THEMES.find(theme => theme.name === config.editor.theme)

if (theme) {
editorTheme.setAttribute('href', `../${theme.path}`)
editorTheme.setAttribute('href', win ? theme.path : `../${theme.path}`)

This comment has been minimized.

Copy link
@AWolf81

AWolf81 May 31, 2019

Author Contributor

@incredibleweirdo Is this working on Mac? Just wondering because if the other change in MarkdownPreview was required then we should change every occurence from ../ to the absolute path.
But if it's working we can also keep it.

This comment has been minimized.

Copy link
@incredibleweirdo

incredibleweirdo May 31, 2019

Yeah, changing themes appears to work fine.

@amedora

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

tested on Win7/64 and confirmed that problems described in #3018 and #3042 has resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.