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

Attribute consolidation #352

Merged
merged 15 commits into from
May 2, 2024
Merged

Attribute consolidation #352

merged 15 commits into from
May 2, 2024

Conversation

JakeWags
Copy link
Member

@JakeWags JakeWags commented Apr 18, 2024

Does this PR close any open issues?

Closes #297 #308 #22 #123

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

This PR consolidates both Deviation and Degree into the attribute list so that the columns can be removed, and eventually universally sorted by. This improves the dynamic UI (see #1 ) and makes the sorting refactor (#123 ) for attributes simpler in the future, as hard coded sort values are now included in the attribute list.

Note: Deviation will always appear as the first item in the attributes list, as it isn't really an attribute, but is handled in a similar way by the dropdown menu.

Bundled in with this is a mini-refactor of attribute header and bar rendering to improve readability and scalability.

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

BEFORE:

upset-attr-consolidation-before

AFTER:

upset-attr-consolidation

Have you added or updated relevant tests?

  • Yes
  • No changes are needed

Have you added or updated relevant documentation?

  • Yes
  • No changes are needed

Are there any additional TODOs before this PR is ready to go?

TODOs:

  • Run tests and confirm alt-text generation when alt-txt-gen #50 is merged and working.

Copy link

netlify bot commented Apr 18, 2024

Deploy Preview for upset2 ready!

Name Link
🔨 Latest commit 6579548
🔍 Latest deploy log https://app.netlify.com/sites/upset2/deploys/6633da9f72890c0008dd0584
😎 Deploy Preview https://deploy-preview-352--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.

@JakeWags JakeWags force-pushed the 297-attribute-consolidation branch from dff5746 to bf76a1d Compare April 24, 2024 17:18
@JakeWags JakeWags marked this pull request as ready for review April 25, 2024 16:25
@JakeWags JakeWags requested a review from NateLanza April 25, 2024 16:25
Copy link
Contributor

@NateLanza NateLanza left a comment

Choose a reason for hiding this comment

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

Most of my comments are related to documentation. I think that, at a minimum, all types and functions need to be documented. In particular, the Props types used to pass arguments to components are important to document as those serve as the only documentation for the component's parameters. I know that's a deviation from the historical norm in this repo but I think we should aim to add documentation as we update the code even if that creates a mismatch in documentation amounts throughout the code.

Also, what's your tab width set to in VSCode? There are a couple spots where it looks like it defaulted to 4 spaces, but most of the repo appears to use 2 spaces.

packages/app/src/components/AttributeDropdown.tsx Outdated Show resolved Hide resolved
packages/app/src/components/AttributeDropdown.tsx Outdated Show resolved Hide resolved
packages/app/src/components/AttributeDropdown.tsx Outdated Show resolved Hide resolved
packages/core/src/aggregate.ts Outdated Show resolved Hide resolved
packages/core/src/sort.ts Outdated Show resolved Hide resolved
packages/upset/src/components/Header/AttributeButton.tsx Outdated Show resolved Hide resolved
packages/upset/src/components/Header/AttributeButton.tsx Outdated Show resolved Hide resolved
packages/upset/src/components/Upset.tsx Show resolved Hide resolved
@JakeWags JakeWags requested a review from NateLanza May 2, 2024 17:41
Copy link
Contributor

@NateLanza NateLanza left a comment

Choose a reason for hiding this comment

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

Two conversations remain unresolved, but neither is worth delaying the PR. Everything else looks good & I especially appreciate all of the added documentation that wasn't specifically requested.

@JakeWags JakeWags changed the title 297 attribute consolidation Attribute consolidation May 2, 2024
@JakeWags JakeWags mentioned this pull request May 2, 2024
4 tasks
@JakeWags JakeWags merged commit ccecd32 into main May 2, 2024
7 checks passed
@JakeWags JakeWags deleted the 297-attribute-consolidation branch May 2, 2024 18:36
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.

Consolidate deviation into other attributes
2 participants