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

UI improvements #43

Merged
merged 15 commits into from
Jan 20, 2024
Merged

UI improvements #43

merged 15 commits into from
Jan 20, 2024

Conversation

pulsejet
Copy link
Member

@pulsejet pulsejet commented Dec 19, 2023

WIP
Testing notes: clear vite cache (node_modules/.vite) and pnpm install again.

@pulsejet pulsejet force-pushed the varun/ui branch 2 times, most recently from 7df4d62 to 46c7398 Compare December 19, 2023 14:34
@pulsejet
Copy link
Member Author

pulsejet commented Dec 19, 2023

One blocking issue here: patch-package doesn't work with pnpm.

EDIT: Well that was easy https://pnpm.io/cli/patch

@@ -108,9 +108,9 @@ registerRoute(/\/stored\/.*/, async (options) => {
});

registerRoute(
/^https:\/\/cdn.jsdelivr.net\/npm/,
/^https?:\/\/(cdn\.jsdelivr\.net|texlive2\.swiftlatex\.com)/,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with caching them but just make sure you know that in the deployed version, LaTeX dependencies are not served by this cache.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm why so?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here I set the path to be our own website:

if (process.env.NODE_ENV && process.env.NODE_ENV !== 'development') {
// Only set URL in production mode
engine.setTexliveEndpoint(`${location.origin}/stored/`)
}

I think we can have more control if we use OPFS. For example, when we want to support something like loading all packages in a zip file, we will be able to do so. Also, I don't think those files will be updated frequently.

}

/** PDF Slick component */
.pdfSlick {
Copy link
Collaborator

Choose a reason for hiding this comment

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

CSS for specific page can be loaded in place:

import cssClassName from './pdfSlick.css'

Then, cssClassName would be a string containing the correct CSS class name.

Copy link
Member Author

@pulsejet pulsejet Dec 21, 2023

Choose a reason for hiding this comment

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

Interesting. I do also want to switch to SCSS asap; writing plain CSS is nightmarish

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's also good.

@zjkmxy
Copy link
Collaborator

zjkmxy commented Jan 15, 2024

You have already pushed too much to this PR and made it very hard to review.
Please stop pushing and mark it ready to review ASAP.
I will close this PR next Friday if it is not ready by then.

@pulsejet
Copy link
Member Author

Ah yes

@pulsejet pulsejet marked this pull request as ready for review January 15, 2024 05:56
@pulsejet
Copy link
Member Author

pulsejet commented Jan 16, 2024

@zjkmxy comments here?

EDIT: btw pls squash if you merge; the history is irrelevant.

@zjkmxy zjkmxy self-assigned this Jan 20, 2024
Copy link
Collaborator

@zjkmxy zjkmxy left a comment

Choose a reason for hiding this comment

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

LGTM

@zjkmxy zjkmxy merged commit 981655e into main Jan 20, 2024
3 checks passed
@zjkmxy zjkmxy deleted the varun/ui branch January 20, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants