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 redo dec 24 #439

Open
wants to merge 56 commits into
base: main
Choose a base branch
from
Open

UI redo dec 24 #439

wants to merge 56 commits into from

Conversation

NateLanza
Copy link
Contributor

@NateLanza NateLanza commented Jan 7, 2025

Does this PR close any open issues?

Closes #427
Closes #422
Closes #421
Closes #403

Give a longer description of what this PR addresses and why it's needed

Makes a large number of changes to the Upset UI:

  • Moves load data into the meatball menu
  • Moves data table to hamburger, includes link to it in the accessibility statement
  • Makes “Settings” sidebar title same style as right sidebars
  • Makes settings collapsible via hamburger icon
  • Changes hamburger in header to gear and re-orders contents
  • Corrects font size for alt text sidebar
  • Hides 2nd agg until 1st agg is selected
  • Makes sure each section title in the settings menu is an h2 with proper weight & size
  • Brings back logo removed for CHI in top left; changes it to the VDL flask
    • Re-adds flask icon in footer
  • Uses emdash between Upset and Vis.. in title
  • Improves vertical centering of attribute col titles
  • Right-aligns toggles in settings sidebar
  • Makes sidebar expand button match close button, on the same size

Provide pictures/videos of the behavior before and after these changes (optional)

(not included as the changes are comprehensively covered above and probably won't fit in a gif)

Have you added or updated relevant tests?

  • Yes
  • No changes are needed

Nate Lanza added 30 commits December 14, 2024 14:39
Copy link

netlify bot commented Jan 7, 2025

Deploy Preview for upset2 ready!

Name Link
🔨 Latest commit 0f090aa
🔍 Latest deploy log https://app.netlify.com/sites/upset2/deploys/6781b1d6776b8c0008bd9faa
😎 Deploy Preview https://deploy-preview-439--upset2.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.

@NateLanza NateLanza self-assigned this Jan 8, 2025
@NateLanza NateLanza marked this pull request as ready for review January 8, 2025 01:14
@NateLanza NateLanza requested review from JakeWags and JackWilb January 8, 2025 01:14
Copy link
Member

@JakeWags JakeWags left a comment

Choose a reason for hiding this comment

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

Some minor changes, overall looks great.

In general and in the future, I would suggest pushing the lint fixes in a separate PR that has no or little new content added. With the linting (for /app) in here, it became very time consuming to differentiate new code/changes that weren't only linting fixes.

e2e-tests/elementView.spec.ts Show resolved Hide resolved
packages/app/.eslintrc.js Outdated Show resolved Hide resolved
packages/app/src/utils/dataTableLink.tsx Outdated Show resolved Hide resolved
packages/app/src/index.css Show resolved Hide resolved
packages/upset/src/atoms/dimensionsAtom.ts Show resolved Hide resolved
packages/upset/src/components/ProvenanceVis.tsx Outdated Show resolved Hide resolved
// Sets the footer height atom if provided as an argument
const setFooterHeight = useSetRecoilState(footerHeightAtom);
// Footer height needs to be doubled to work right... idk why that is!
useEffect(() => { if (footerHeight) setFooterHeight(2 * footerHeight); }, [footerHeight]);
Copy link
Member

Choose a reason for hiding this comment

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

If you were to mult by 1 and simply double the variable value, would it work? Curious to see what could be causing this type of behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wdym by this? How is multiplying by 1 and doubling any different from multiplying by 2?

Copy link
Member

Choose a reason for hiding this comment

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

It's not any different, I am just curious if there is some strange order of operations that necessitates the footerHeight being doubled.

packages/upset/src/components/SettingsSidebar.tsx Outdated Show resolved Hide resolved
@NateLanza
Copy link
Contributor Author

In general and in the future, I would suggest pushing the lint fixes in a separate PR that has no or little new content added. With the linting (for /app) in here, it became very time consuming to differentiate new code/changes that weren't only linting fixes.

Absolutely, I did not process that that'd be an issue. Will do in the future.

@NateLanza NateLanza requested a review from JakeWags January 10, 2025 21:35
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.

Add size labels settings Sets and attributes menues Unify side-windows Add set sizes to plot
2 participants