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

One entry for components #1099

Merged
merged 6 commits into from
Aug 15, 2024
Merged

One entry for components #1099

merged 6 commits into from
Aug 15, 2024

Conversation

hanbyul-here
Copy link
Collaborator

@hanbyul-here hanbyul-here commented Aug 8, 2024

Related Ticket: #1089

Description of Changes

Make one entry file for all the components that are used in an instance. I thought about using library entry file, but some of the components will interrupt the library building, and I don't really want to blur the scope of this PR.

Notes & Questions About Changes

This idea was really nice in my brain.. but now that I see the changes, I am not sure if this would be a better way to use the components from VEDA UI or not. It is surely nice that we don't have to worry about moving components around in the source code. But I'm not sure if importing literally everything from veda-ui-scripts is a good idea or not - We can try to have more structured exports, but I don't really want to add any maintenance efforts of this file. This is how it looks like in the instance - it makes the code look a bit unorganized. But it may be fine?: US-GHG-Center/veda-config-ghg@develop...US-GHG-Center:veda-config-ghg:use-etnry-for-instance

Can y'all give me your thoughts? @sandrahoang686 @dzole0311 Also @slesaad - How do you think about importing all the components needed from one entry file like this? US-GHG-Center/veda-config-ghg@develop...US-GHG-Center:veda-config-ghg:use-etnry-for-instance#diff-546348a53a9dc4b9a8f244b185ac088f93fe7a5573cfc39617f7e8bd9a471d00R27

Copy link

netlify bot commented Aug 8, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 3f8a101
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/66bbbc4aaf11710008a644d3
😎 Deploy Preview https://deploy-preview-1099--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.

@slesaad
Copy link
Member

slesaad commented Aug 8, 2024

Ooo I definitely like this idea!!! 😍

@dzole0311
Copy link
Collaborator

dzole0311 commented Aug 9, 2024

Thanks for the showcase @hanbyul-here !

Personally, I tend to lean toward having more structured exports to keep clear context on where utils, types, and components originate from (assuming tree-shaking is not an issue if having one entry file). For example, if I'm working on an instance and I'm given an option to import "TimeDensity", I’d want to immediately know if it’s a component or a type. With a single entry file I feel like that distinction might be obscured (e.g. import TimeDensity from '$veda-ui-scripts').

I was wondering if it's possible to have a few entry files instead of just one? For example: styles, types, utils and components (the current sub-directory structure we have). This way, on the instance side, rather than doing:

import { FoldProse } from "$veda-ui-scripts/components/common/fold"

we could offer a simplified import structure like:

import { FoldProse } from "$veda-ui-scripts/components"

I think this would still provide a cleaner import experience.

However if having a single entry file simplifies how devs work with veda-ui on the instance side in the near term, I don't see this approach as a blocker.

@sandrahoang686
Copy link
Collaborator

sandrahoang686 commented Aug 9, 2024

Thanks for the explanation and changes @hanbyul-here !

I actually like this way of doing it and its very similar (if not mostly the same) to how we are exposing the components for the new refactor. I think this is simple enough on veda-ui's end to make sure all the components are exported that will be needed, just tedious work to update these on the instances side but its a better pattern I think for sure. For me, I actually wouldn't worry to much technically about how and reducing maintenance because its solution that will exist until the instances migrate to the new architecture which will be some time but this is still temporary. So I think its good enough to move forward with and not spend so much time when it will go away. My 2 cents. Plus later with this entry point file, it will be a big positive and really easy to figure out what the instances depend on when migrating them over 😄 🙌🏼 (instead of having to look on each instance repo)

@hanbyul-here hanbyul-here marked this pull request as ready for review August 13, 2024 21:02
@hanbyul-here
Copy link
Collaborator Author

Ok let's move forward with one entry file for now. I think more structured imports are a really good idea, but I feel like people who develop on the instance level will appreciate the easy access more than the structured imports. 🤔 We can def evolve it later (but it will be even better if the instance can just use UI as a library - in short future 😅 )

@hanbyul-here hanbyul-here merged commit 551cb5e into main Aug 15, 2024
12 checks passed
@hanbyul-here hanbyul-here deleted the one-entry-for-components branch August 15, 2024 12:50
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.

4 participants