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

Enable offline functionality for frontend #6152

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

saichander17
Copy link
Contributor

@saichander17 saichander17 commented Oct 10, 2024

Summary

This PR removes functionality which downloads fonts/css files from internet and puts them available in the local build itself. The UI seems to call other domains as well but they don't block the UI until timeout occurs and hence it's not causing any functionality issues.

Problems this PR solves in airgapped environments

  1. Long waiting time to load UI as it waits for fonts and css file requests to timeout
  2. Broken charts due to uPlot.min.css file not being available
  3. Incorrect fonts being shown as fonts couldn't be downloaded

Related Issues / PR's

Fix for: #5036

External domains still being called from the frontend

These domains still get called from the UI but they don't block the UI until timeout or anything as per my testing.

  1. https://analytics-cdn.signoz.io/
  2. https://widget.intercom.io/
  3. https://api.github.com/repos/signoz/signoz/releases/latest
  4. https://js.intercomcdn.com/

Known problems

  1. Monaco editor doesn't load to import JSON (load monaco from node_modules suren-atoyan/monaco-react#12). I'm not sure if I should follow the steps mentioned in this issue or is there any problem with the approach.

Clarifications needed

  • I've placed uPlot.min.css under public/css and imported in the index html. Should I do it this way or import it in the src/styles.scss similar to how it's done for periscope.scss?

My suggestion would be to go ahead and merge the PR as it at least solves the biggest issues in airgapped environments right now and take other updates periodically.


Important

Enable offline functionality by localizing fonts and CSS in the frontend, addressing airgapped environment issues.

  • Behavior:
    • Removes external font and CSS file requests in index.html.ejs, replacing them with local paths.
    • Adds uPlot.min.css to public/css and updates index.html.ejs to use it locally.
  • Styles:
    • Adds @font-face declarations in styles.scss for Inter, Work Sans, Space Mono, and Fira Code fonts, sourcing them from local files.
  • Misc:
    • Updates image paths in index.html.ejs to use local images instead of external URLs.

This description was created by Ellipsis for 2a1e8da. It will automatically update as commits are pushed.

Copy link

welcome bot commented Oct 10, 2024

Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗

@CLAassistant
Copy link

CLAassistant commented Oct 10, 2024

CLA assistant check
All committers have signed the CLA.

@vikrantgupta25 vikrantgupta25 self-assigned this Oct 14, 2024
@saichander17 saichander17 marked this pull request as ready for review October 14, 2024 05:51
@saichander17 saichander17 requested a review from YounixM as a code owner October 14, 2024 05:51
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 2a1e8da in 27 seconds

More details
  • Looked at 90 lines of code in 3 files
  • Skipped 5 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/index.html.ejs:55
  • Draft comment:
    The external font links have been removed, but the HTML does not reference the locally defined fonts. Ensure that the HTML or CSS uses the locally defined fonts from styles.scss.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. frontend/src/styles.scss:277
  • Draft comment:
    Avoid hardcoding color values like rgb(136, 136, 136). Use design tokens or predefined color constants instead. This applies to other color values in this file as well.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_9obqgpOwqsvQ2Ek3


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

frontend/public/css/uPlot.min.css Show resolved Hide resolved
@vikrantgupta25
Copy link
Member

the changes look good, can you attach a video recording with launching query service locally / frontend locally and turn off the internet access. Load certain dashboards and check if things are looking fine ?

Copy link
Member

@vikrantgupta25 vikrantgupta25 left a comment

Choose a reason for hiding this comment

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

LGTM

@vikrantgupta25 vikrantgupta25 enabled auto-merge (squash) October 23, 2024 05:49
@YounixM YounixM disabled auto-merge October 23, 2024 05:50
@YounixM YounixM enabled auto-merge (squash) October 23, 2024 05:51
@YounixM YounixM merged commit 347868c into SigNoz:develop Oct 23, 2024
11 of 13 checks passed
Copy link

welcome bot commented Oct 23, 2024

Congrats on merging your first pull request!
minion-party
We here at SigNoz are proud of you! 🥳

@prashant-shahi
Copy link
Member

prashant-shahi commented Oct 28, 2024

@saichander17 Can you please sign the CLA? https://cla-assistant.io/SigNoz/signoz?pullRequest=6152

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.

6 participants