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

refactor: Clean up golden-layout css #1338

Merged
merged 3 commits into from
Jun 2, 2023
Merged

Conversation

mattrunyon
Copy link
Collaborator

@mattrunyon mattrunyon commented May 31, 2023

Reverts #1334

Applies #1322 w/ a fix for enterprise builds

Still seems to need the ignore in karma.config.cjs though. Probably because the golden-layout package tests use old libraries

@mattrunyon mattrunyon self-assigned this May 31, 2023
Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

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

Was the change to simply move it adjacent to the .js rather than in the /scss folder?

@mattrunyon
Copy link
Collaborator Author

mattrunyon commented May 31, 2023

Also had to add ./src:./dist to the build:sass for golden-layout so that the module would be compiled to GoldenLayout.module.css

The reference to ../scss would always point to scss, but we convert the imports to css for distributed files. The only scss files we publish are the base stylesheet/themes as those are intended to be consumed in other scss files in enterprise/code-studio

Comment on lines +28 to +31
// ignored because it doesn't understand the scss import in GoldenLayoutThemeExport
configure: function (bundle) {
bundle.on('prebundle', function () {
bundle.ignore('./dist/GoldenLayoutThemeExport.js');
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I full understand this. The ./dist/GoldenLayoutThemeExport.js file that's outputted has a css import, not an scss import - so what's being ignored here specifically?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was from the original PR, but I think it is skipping this file instead of trying to bundle it with browserify which I guess doesn't like the CSS import? Idk, but I tried removing it and the tests failed.

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

@dsmmcken Are all the updates screenshots correct? Looks like things shifted horizontally by a couple pixels

@mattrunyon
Copy link
Collaborator Author

The screenshots should be from the original PR. I just reverted the revert commit then added the fix on top of that.

@dsmmcken
Copy link
Contributor

dsmmcken commented Jun 2, 2023

Screenshots are correct.

  1. drag proxy tab right border edge was changed.
  2. overflow of placeholder text was fixed.
    image

@dsmmcken dsmmcken merged commit 210908c into deephaven:main Jun 2, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2023
@mattrunyon mattrunyon deleted the 1322-fix branch June 23, 2024 06:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants