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

chore: Remove veda dependency #1398

Merged
merged 3 commits into from
Jan 23, 2025
Merged

Conversation

hanbyul-here
Copy link
Collaborator

@hanbyul-here hanbyul-here commented Jan 22, 2025

Related Ticket: #1360

This PR removes VEDA dependency from the registry build. Run yarn buidliblocally, see how less number of assets it builds to test it. (PageHeader and PageFooter were the root of the cause)

Screenshot 2025-01-22 at 3 33 43 PM

Compare it to : #1367

It is kind of difficult to think what kind of test we can do check this, but it will be nice to have some automatic checks - since it is much easier to get rid of veda dependency when you know what directly introduced it.

Copy link

netlify bot commented Jan 22, 2025

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit c2b94af
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/679244b5bc430900087b0382
😎 Deploy Preview https://deploy-preview-1398--veda-ui.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.

@hanbyul-here hanbyul-here changed the title Remove veda dependency chore: Remove veda dependency Jan 22, 2025
Copy link
Collaborator

@sandrahoang686 sandrahoang686 left a comment

Choose a reason for hiding this comment

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

I don't think VedaUIProvider is the best place, this provider has been a workaround for us to handle env vars and workarounds for framework related things (routing). It would be great if we could keep the usecase for it limited.

I also think we dont need it as part of context and could just isolate to the hook. It looks like pretty simple logic where all its being used for is to just show/not show something so this could be entirely simplified. Really, we dont even need the hook but created a PR to this branch here to make said changes if you could take a look (this should also fix the tests because no more dep on provider).

@hanbyul-here hanbyul-here merged commit 5768355 into main Jan 23, 2025
11 checks passed
@hanbyul-here hanbyul-here deleted the 1360-remove-veda-dependency branch January 23, 2025 15:01
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.

2 participants