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

Add feature: Display Point & RothC output #56

Merged
merged 7 commits into from
Aug 20, 2021
Merged

Add feature: Display Point & RothC output #56

merged 7 commits into from
Aug 20, 2021

Conversation

shubhamkarande13
Copy link
Member

@shubhamkarande13 shubhamkarande13 commented Aug 16, 2021

Description

Displays the received Point and RothC outputs as a grid/spreadsheet.

Introducing RevoGrid for rendering the outputs as a spreadsheet.

Point-RothC-spreadsheet.mp4

@aornugent
Copy link
Collaborator

Great start! I'm interested to see it with real data. 💯

@shubhamkarande13
Copy link
Member Author

shubhamkarande13 commented Aug 18, 2021

@aornugent I've noticed that point.results and rothc.results point to the exact same variable results in the store, and hence when I run the Point module, the RothC result also gets updated. So the modular approach of the state is just for readability, and doesn't actually isolate all the different variables.
Can we go back to using different names for all state variables?

image

@shubhamkarande13 shubhamkarande13 marked this pull request as ready for review August 18, 2021 17:21
@shubhamkarande13
Copy link
Member Author

@waridrox could you please test to check if the configuration is broken or not? Everything seems fine at my end.

@shubhamkarande13
Copy link
Member Author

Please have a look at the feature demo in the first comment!

@aornugent
Copy link
Collaborator

aornugent commented Aug 18, 2021

Great work @shubhamkarande13! A few thoughts:

  • that's disappointing our module namespacing isn't working correctly. It might be that we need to be more selective about how we load the Vuex modules. They should be tied to specific components rather than loaded into the global namespace. Please file an issue describing how the code works now and how it's intended to work. Then we have a space to research the problem more fully before implementing a fix.
  • the table is a little hard to read, it probably needs a white background and some borders. It's unusual to have it take up the whole page for just a few columns. Again, if you're not interested in fixing it in this PR, then leave a comment on Add a table component #4 for future reference.
  • somehow your GCBM changes on add feature: create new GCBM job! #54 have made it into this PR, please remove them.

@shubhamkarande13
Copy link
Member Author

shubhamkarande13 commented Aug 19, 2021

@aornugent could you please review the PR once again? I tried to drop the GCBM job commit from this branch. 😅

@waridrox could you please beautify the tables a bit as suggested by @aornugent ? I'm bad at design. 😂

Thank you both! 🤗♥️

@waridrox
Copy link
Member

Here's the preview for modified implementation. The state updation from vuex could be referenced later, though it is a crucial thing to worry about 😕 - https://michaelnthiessen.com/debugging-guide-why-your-component-isnt-updating/

graph-demo.mov

@aornugent
Copy link
Collaborator

aornugent commented Aug 20, 2021

@waridrox - agreed, it's important. I like the mapState pattern in the link you shared above. I believe this can be used to handle modules that share similar names by setting namepace: true, as described here:

https://laracasts.com/discuss/channels/vue/vuex-actions-with-same-name-in-different-modules?page=0

But I don't yet understand why we have conflict given that our modules are isolated within components.

Is it true that store/point.js is only loaded in point components (and store/rothc.js is only loaded in rothc components) or are they being invoked at a higher level that causes a namespace collison?

This PR is looking good for now, so let's file an issue to continue the namespace discussion after we merge.

@waridrox waridrox merged commit af9b4a4 into master Aug 20, 2021
@iamrajiv iamrajiv deleted the testTables branch September 6, 2021 15:57
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.

5 participants