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

Remove css-in-js button styles from core #139

Merged
merged 7 commits into from
Mar 14, 2024
Merged

Conversation

dmijatovic
Copy link
Collaborator

@dmijatovic dmijatovic commented Mar 8, 2024

Based on the feedback from users of haddock web app and the investigation performed in this issue it is been decided to remove css-in-js styles from core library. Instead, the specific classes are added which can be used for customization btn-catalog-node, btn-visual-node, btn-visual-panel.

It was not possible to completely remove @emotion dependency. There were build errors with the storybook components. I moved @emotion to devDependencies for now.

After removing styles from core lib, the (example) download app styles are extended. Additional (similar) work might be needed in the other apps (I have not checked it actually).

Example download app (with changed styles)

image

…pendecy seem to be required by storybook, so it is moved to devDependencies
Copy link

netlify bot commented Mar 8, 2024

Deploy Preview for i-vresse-workflow-builder ready!

Name Link
🔨 Latest commit 41b32a2
🔍 Latest deploy log https://app.netlify.com/sites/i-vresse-workflow-builder/deploys/65f2daff0bcbb10008705b07
😎 Deploy Preview https://deploy-preview-139--i-vresse-workflow-builder.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 18.18182% with 45 lines in your changes are missing coverage. Please review.

Project coverage is 67.04%. Comparing base (7206c03) to head (41b32a2).
Report is 1 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #139      +/-   ##
==========================================
+ Coverage   66.74%   67.04%   +0.29%     
==========================================
  Files          57       57              
  Lines        4065     4017      -48     
  Branches      337      337              
==========================================
- Hits         2713     2693      -20     
+ Misses       1348     1320      -28     
  Partials        4        4              
Flag Coverage Δ
core-unit 64.12% <18.18%> (+0.33%) ⬆️
form-unit 78.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
packages/core/src/constants.ts 100.00% <ø> (ø)
packages/core/src/Example.tsx 0.00% <0.00%> (ø)
packages/core/src/VisualNode.tsx 21.05% <66.66%> (-17.95%) ⬇️
packages/core/src/CatalogNode.tsx 0.00% <0.00%> (ø)
packages/core/src/VisualPanel.tsx 66.07% <33.33%> (+5.00%) ⬆️
packages/core/src/WorkflowPanel.tsx 0.00% <0.00%> (ø)

Copy link
Member

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

I was able to copy the build of core package to haddock3 webapp, it worked but missing styles, but that can be picked up in a pr on the webapp repo.

The tables in guru catalog > rigidbody > symetry now have a horizontal scrollbar on themselves instead of whole form. Which is better, but you now have to scroll each time you want to add a row (see #141)

The download button disappears under the page fold when the form gets expanded a lot.
while before a scrollbar was shown around the form/catalog/workflow. In the hadock3 webapp the button row floats. Having the least amount of scrollbars is I think my preference, Not sure if the button row should be sticky or get pushed down. What do you think?

I approved this PR as inline comments can be handled in this PR or later.

}

/* Show panel section is clickable */
h5[id^="expander4"]{
Copy link
Member

Choose a reason for hiding this comment

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

Can we use .panel-title class instead of id begins with selector? Or is the class used elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"panel-title" class is not used in all panels :-(. This is the only selector I found that works for all collapsible panels.

apps/kitchensink/src/kitchensink.json Show resolved Hide resolved
apps/haddock3-submit/src/App.css Show resolved Hide resolved
apps/haddock3-download/src/App.css Show resolved Hide resolved
@dmijatovic dmijatovic merged commit ddc0190 into main Mar 14, 2024
5 checks passed
@dmijatovic
Copy link
Collaborator Author

dmijatovic commented Mar 14, 2024

The download button disappears under the page fold when the form gets expanded a lot. while before a scrollbar was shown around the form/catalog/workflow. In the hadock3 webapp the button row floats. Having the least amount of scrollbars is I think my preference, Not sure if the button row should be sticky or get pushed down. What do you think?

Maybe we can move the global parameters to be first item in the workflow list (without the option for move and delete) and then have three workflow buttons at the top: Upload, Download and Clear. For the "node" buttons (save, delete and cancel) we could implement some basic "auto save" approach and remove the buttons. Delete option is present in the list but we loose the cancel option in this case.

@sverhoeven sverhoeven mentioned this pull request May 1, 2024
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.

3 participants