Skip to content

[MINOR] broken logo url in web dev mode #1892

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.

Sign up for GitHub

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

Jump to bottom
Closed
soralee wants to merge 4 commits into from
Closed

[MINOR] broken logo url in web dev mode #1892

soralee wants to merge 4 commits into from

Conversation

@soralee
Copy link
Contributor

@soralee soralee commented Jan 12, 2017 •
edited
Loading

What is this PR for?

zeppelin logo image (zepLogo.png) is broken in web dev mode like below screenshot.
screenshot from 2017-01-12 14-50-35

What type of PR is it?

[Bug Fix]

What is the Jira issue?

How should this be tested?

  1. Run front-end application in dev mode
  2. Open console in web developer mode.
  3. Connect/refresh localhost:9000
  4. Check to be shown Logo image in Zeppelin home.
    image
  5. Check errors in console.

Screenshots (if appropriate)

screenshot from 2017-01-12 16-58-40

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@soralee soralee force-pushed the ZEPPELIN-1943_logo branch from d56b36f to 19c5eb8 Compare January 12, 2017 09:10
@minahlee
Copy link
Member

minahlee commented Jan 12, 2017

This fix will bring same issue in production mode if user set ZEPPELIN_SERVER_CONTEXT_PATH. Do you have any other idea to work around?

@soralee
Copy link
Contributor Author

soralee commented Jan 13, 2017 •
edited
Loading

Thanks for review and considering the side effect @minahlee 👍
You're right! I missed this option.

As your mention, I checked ZEPPELIN_SERVER_CONTEXT_PATH option.

Test 1. When I set ZEPPELIN_SERVER_CONTEXT_PATH to home and connect web dev mode (localhost:9000/home), it is okay.
z_icon_9000

Test 2. But, when I set ZEPPELIN_SERVER_CONTEXT_PATH to home and connect localhost:8080/home`, there is the problem 😢
z_icon_8080

I'll update it and inform to you after resolving this issue.

@soralee soralee force-pushed the ZEPPELIN-1943_logo branch from 19c5eb8 to 4185b47 Compare January 18, 2017 07:45
@soralee
Copy link
Contributor Author

soralee commented Jan 18, 2017

@minahlee I updated and resolved another way.
When web application develop mode, the source base of production mode is different to web application develop mode. So, I updated to correct source base path for develop mode of web application.

@soralee soralee force-pushed the ZEPPELIN-1943_logo branch 3 times, most recently from aedd691 to e9cc2c3 Compare January 23, 2017 05:58
@Leemoonsoo
Copy link
Member

Leemoonsoo commented Jan 26, 2017

cc @1ambda

@1ambda
Copy link
Member

1ambda commented Feb 1, 2017

Sorry for late reply. I will review and comment ASAP @soralee

1ambda
1ambda reviewed Feb 2, 2017
View reviewed changes
Copy link
Member

@1ambda 1ambda left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@soralee works well! I left few comments. it would be great if they are resolved as well before merging :)

Additionally, the real problem is that we don't have public directory. This makes a lot of weird webpack configurations (not only static).

I hope we can also solve this problem soon.

Anyway, thanks for caring build tools in zeppelin-web @soralee !

zeppelin-web/webpack.config.js Outdated
'/bower_components/',
require('express').static(path.join(__dirname, './bower_components/')));
app.use('**/bower_components/',
require('express').static(path.resolve(__dirname, './bower_components/')));
Copy link
Member

@1ambda 1ambda Feb 2, 2017

Choose a reason for hiding this comment

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

@soralee I think we can extract require('express') as variable. But it's ok not to modify. it's trivial :)

Copy link
Contributor Author

@soralee soralee Feb 2, 2017

Choose a reason for hiding this comment

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

Thanks @1ambda for review! Yes, it makes sense! Let me updated this part ASAP :)

zeppelin-web/webpack.config.js Outdated
require('express').static(path.join(__dirname, './bower_components/')));
app.use('**/bower_components/',
require('express').static(path.resolve(__dirname, './bower_components/')));
app.use('**/app/', require('express').static(path.resolve(__dirname, './src/app/')));
Copy link
Member

@1ambda 1ambda Feb 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think we don't need app, components, fonts in static. since they are already served using relative paths.

Updated: serving app, component and fonts dirs in static is necessary for supporting nested context path like /a/b as @soralee originally intended.

Copy link
Member

@1ambda 1ambda Feb 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

But if we use nested context paths, it cause another problems like the screenshot below. So i believe having the correct directory structure for static files is the solution to solve these kind of errors. However, it can be handed in other PRs.

image

Copy link
Contributor Author

@soralee soralee Feb 2, 2017

Choose a reason for hiding this comment

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

Actually I missed this issue in production mode.
Yes! I totally agree with @1ambda. I think we need reorganize directory to resolve kind of this issue in correct way.

@soralee soralee force-pushed the ZEPPELIN-1943_logo branch from e9cc2c3 to 7819871 Compare February 2, 2017 08:01
@soralee
Copy link
Contributor Author

soralee commented Feb 2, 2017

@1ambda I updated to extract require('express') module as variable and CI is green :)

@1ambda
Copy link
Member

1ambda commented Feb 2, 2017

LGTM!

@asfgit asfgit closed this in b8600a6 Feb 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@1ambda 1ambda 1ambda left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@soralee @minahlee @Leemoonsoo @1ambda