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

[Dashboard] Remove <AspectRatio/> #4750

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

kien-ngo
Copy link
Contributor

@kien-ngo kien-ngo commented Sep 22, 2024

Problem solved

Short description of the bug fixed or feature added


PR-Codex overview

This PR focuses on removing the AspectRatio component from various files and replacing its usage with div elements styled with Tailwind CSS classes to achieve the same aspect ratio effect.

Detailed summary

  • Removed AspectRatio imports from multiple components.
  • Replaced AspectRatio usage with div elements using Tailwind CSS classes for aspect ratios.
  • Updated related prop types to reflect the removal of AspectRatio.
  • Adjusted CSS classes for styling consistency across components.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

changeset-bot bot commented Sep 22, 2024

⚠️ No Changeset found

Latest commit: 28f128d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Sep 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 1:37am
thirdweb_playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 1:37am
thirdweb-www ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 1:37am
wallet-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 1:37am

@vercel vercel bot temporarily deployed to Preview – docs-v2 September 22, 2024 19:56 Inactive
@vercel vercel bot temporarily deployed to Preview – thirdweb_playground September 22, 2024 19:56 Inactive
@vercel vercel bot temporarily deployed to Preview – wallet-ui September 22, 2024 19:56 Inactive
Copy link

graphite-app bot commented Sep 22, 2024

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@github-actions github-actions bot added the Dashboard Involves changes to the Dashboard. label Sep 22, 2024
@kien-ngo kien-ngo changed the title Remove <AspectRatio/> [Dashboard] Remove <AspectRatio/> Sep 22, 2024
@kien-ngo kien-ngo marked this pull request as ready for review September 22, 2024 19:56
Copy link

codecov bot commented Sep 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.11%. Comparing base (e2b3484) to head (28f128d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4750   +/-   ##
=======================================
  Coverage   49.11%   49.11%           
=======================================
  Files        1052     1052           
  Lines       56786    56786           
  Branches     3889     3889           
=======================================
  Hits        27892    27892           
  Misses      28261    28261           
  Partials      633      633           
Flag Coverage Δ *Carryforward flag
legacy_packages 65.68% <ø> (ø) Carriedforward from e2b3484
packages 45.19% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

Copy link
Contributor

github-actions bot commented Sep 22, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
thirdweb (esm) 41.91 KB (0%) 839 ms (0%) 2.4 s (-8.04% 🔽) 3.2 s
thirdweb (cjs) 101.47 KB (0%) 2.1 s (0%) 6.5 s (+8.89% 🔺) 8.6 s
thirdweb (minimal + tree-shaking) 4.82 KB (0%) 97 ms (0%) 315 ms (+22.87% 🔺) 411 ms
thirdweb/chains (tree-shaking) 498 B (0%) 10 ms (0%) 222 ms (+253.93% 🔺) 232 ms
thirdweb/react (minimal + tree-shaking) 16.73 KB (0%) 335 ms (0%) 804 ms (-20.76% 🔽) 1.2 s

@@ -1,4 +1,3 @@
import { AspectRatio } from "@/components/ui/aspect-ratio";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jnsdls I didn't know there was another AspectRatio so I mistakenly removed this one. And now the lint is telling me due to this change, @radix-ui/react-aspect-ratio is no longer being used.

So, should we remove said dependency, or use in as the replacement for all the Chakra-ui's AspectRatio?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I have a feeling you would want to remove the dep)

Copy link
Member

Choose a reason for hiding this comment

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

@MananTank had thoughts on this last time it came up, will have him make the decision on what we do here, but either way it's either we use css or we use the component, we have to pick one of the options, not both

Copy link
Member

@MananTank MananTank Sep 23, 2024

Choose a reason for hiding this comment

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

We can replace all AspectRatio components with CSS/tailwind class but my only concern is that it's not a an exact replacement, (I've seen this when I refactored the FileInput component - I had to make some other changes to make the aspect-ratio work exactly as chakra version ).

Lets verify that all changes are working as expected and we are not accidentally breaking the layout. If they are are - feel free to remove all the components with css class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MananTank yeah it's not working exactly the same you're right. I had to make few minor changes to re-align the inner content (re-center, more like).

I guess that means we stick with @radix-ui/react-aspect-ratio

Copy link
Member

Choose a reason for hiding this comment

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

Let's replace the instances where it works with just a css class and make changes in other places as needed and remove AspectRatio component

@vercel vercel bot temporarily deployed to Preview – thirdweb_playground September 22, 2024 20:27 Inactive
@vercel vercel bot temporarily deployed to Preview – wallet-ui September 22, 2024 20:27 Inactive
@vercel vercel bot temporarily deployed to Preview – docs-v2 September 22, 2024 20:27 Inactive
@jnsdls jnsdls changed the base branch from kien/remove-hstack to graphite-base/4750 September 23, 2024 01:30
@vercel vercel bot temporarily deployed to Preview – wallet-ui September 23, 2024 01:42 Inactive
@vercel vercel bot temporarily deployed to Preview – docs-v2 September 23, 2024 01:42 Inactive
@vercel vercel bot temporarily deployed to Preview – thirdweb_playground September 23, 2024 01:42 Inactive
@vercel vercel bot temporarily deployed to Preview – thirdweb_playground September 24, 2024 10:07 Inactive
@vercel vercel bot temporarily deployed to Preview – wallet-ui September 24, 2024 10:07 Inactive
@vercel vercel bot temporarily deployed to Preview – docs-v2 September 24, 2024 10:07 Inactive
@vercel vercel bot temporarily deployed to Preview – docs-v2 September 24, 2024 15:08 Inactive
@vercel vercel bot temporarily deployed to Preview – wallet-ui September 24, 2024 15:08 Inactive
@vercel vercel bot temporarily deployed to Preview – thirdweb_playground September 24, 2024 15:08 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dashboard Involves changes to the Dashboard.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants