CenterForTheBuiltEnvironment / ABCWeb

Web Interface for ABC
0 stars 0 forks source link

Enable MRT JSON upload #21

Closed xyntechx closed 1 year ago

xyntechx commented 1 year ago

Solves #6

xyntechx commented 1 year ago

Yep! I made this the same way as how Brian made it for the customizable inputs for each body segment. So if we want to make this more robust, we should make the existing code more robust as well. This can be done by storing the data in an object instead of array state, or separate each body segment into its own state. The latter would make the code more readable and maintainable as well :))

xyntechx commented 1 year ago

While we're thinking of making the code more robust, may I suggest decomposing the code into separate components? I feel that the index.js file is too big, and it would be a better dev experience if we separated different segments into different components. Also, I think the main state that the code is using is quite convoluted and stores many different variables, which is not exactly the best practice for performance and dev experience. Decomposing index.js would help with this as well as we would need to break the big state down into simpler, more singular states.

charliehuizenga commented 1 year ago

Absolutely. Up to this point I have not been reviewing the code but would like to start doing that so we can be building more robust, better organized code. Would be great if you could make a proposal for a reorganization.

thanks!

On Fri, Oct 20, 2023 at 9:29 PM Nyx Iskandar @.***> wrote:

While we're thinking of making the code more robust, may I suggest decomposing the code into separate components? I feel that the index.js file is too big, and it would be a better dev experience if we separated different segments into different components. Also, I think the main state that the code is using is quite convoluted and stores many different variables, which is not exactly the best practice for performance and dev experience. Decomposing index.js would help with this as well as we would need to break the big state down into simpler, more singular states.

— Reply to this email directly, view it on GitHub https://github.com/CenterForTheBuiltEnvironment/ABCWeb/pull/21#issuecomment-1773656118, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMNCCB5EOUILATQ3O2YJGXTYANFTZAVCNFSM6AAAAAA6JL7YPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZTGY2TMMJRHA . You are receiving this because your review was requested.Message ID: @.***>

briansongdev commented 1 year ago

I agree with both of you, the code definitely can be refactored into cleaner components. I'll also get working on that and Nyx let's communicate in chat about which components to take care of so we're on the same page. Thanks!

xyntechx commented 1 year ago

Sure! We can make a separate branch rewriting the code :))

briansongdev commented 1 year ago

Sure! I'll go ahead and start on decomposing the file into smaller components so it's more readable.

xyntechx commented 1 year ago

Okay! I guess to avoid double-work I can just help to review your code once you're done. Lmk when you've made a PR and I can see what else I can help improve :)

briansongdev commented 1 year ago

Hey Nyx, I cut down the main file's code by roughly 60%, separating what I thought could be grouped together into different files mainly under the two new folders, components and constants. Let me know what you think and if you feel like some of the variables in index.js could be better named, feel free to do that as well! I'm not the best at naming conventions, as can be demonstrated by my code 😆