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

Web client improvement #815

Closed
wants to merge 5 commits into from

Conversation

PeterDaveHello
Copy link
Contributor

This PR contains changes as below:

  • Update date 3-party libraries
  • Clean up dead files
  • Fix wrong file permission
  • Unify coding style in index.html (using 4 spaces, can be revised)

Please let me know if there is anything needs to be changed here, thanks!

@PeterDaveHello
Copy link
Contributor Author

@livings124 @mikedld would you like to take a look? Thanks.

@PeterDaveHello
Copy link
Contributor Author

@ckerr can you help review this one? Thanks.

Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

👍 on most of the ideas here. Some suggestions:

  • Even though the end result would be the same, I'd prefer to see these separate topics broken into their own PRs since they're unrelated. For example, the jQuery / jQuery UI bumps are not related to the indentation styling, and neither are related to the index.html exec flag bugfix. So those all belong in separate PRs.

  • The jQuery UI refresh grew the png sizes. I think previously what we did was run them through pngcrush or pngquant to shrink them down ... could you do that on these new icons as well to remove that new overhead?

  • I'm not in love with the idea of removing the source versions of the minified files. IIRC we don't bundle them into our release -- and if we do, I'd 👍 a patch that stopped that from happening -- but it's still useful to have them in the repo for reference.

@PeterDaveHello
Copy link
Contributor Author

@ckerr thanks for the comment, I've updated the PR, please take a look ;)

@PeterDaveHello
Copy link
Contributor Author

@ckerr do you have a minute to take a look at the revised result? Thanks a lot.

@ckerr
Copy link
Member

ckerr commented Oct 7, 2021

I realize how long this PR has been in limbo and am trying to clear through some of the backlog. This PR is no longer applicable because jQuery is no longer used in the web client as of #1476.

@ckerr ckerr closed this Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants