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

Milka & Amelie Issue #1150 #1171

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

Milka & Amelie Issue #1150 #1171

wants to merge 1 commit into from

Conversation

MilkaZek
Copy link
Collaborator

We added a function in update-app.js to reset the Demo Dropdown, and call it when the Generate Network button is clicked, which seems to have fixed the bug of the original demo not being able to be reclicked after generating a network. There is a possibility that this code can be condensed/placed in a different place, but we were unsure about trying that out.

@dondi dondi changed the base branch from main to beta February 12, 2025 16:45
Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

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

So after going a bit deeper into the existing code, I think I’ve put my finger on the core underlying issue: grnState doesn’t have a notion of “selected demo.” The idea of the selected demo is currently housed exclusively in the value of #demoSourceDropdown. To make this follow our MVC flow cleanly, I think we should start with that change, then take it from there

As such, I’d say the steps are as follows:

  1. Add a new property to grnState (grnstate.js) which stores the currently-selected demo. This can be none when the current network is from a different source
  2. Somewhere in updateApp, we need a block of code that updates #demoSourceDropdown based on this new property in grnState. We can also update the other items as pointed out by @ntran18 (the menu bar and the demo menu) at this point, because they are all based on this new grnState property
  3. Existing code that touches #demoSourceDropdown (including what’s in this PR) should then be restructured such that:
    • They modify the grnState property (look at existing code for how this is done by other UI elements)
    • This change to grnState should then invoke updateApp…I’m not sure at this point if this update is done implicitly or if it has to be explicitly called; check existing code to see how the cycle goes

I realize that this is now a lot more work than what you have in this PR 😅 but the idea is to implement the fix within the proper structure as well. It is fair to say that this bug actually exists precisely because the value of the selected demo was not stored in grnState to begin with; had that property been tracked in that centralized way, the original question of where to put this code would have been easier to answer

@ntran18 @ceciliazaragoza how does the plan above sound?

@ceciliazaragoza
Copy link
Collaborator

That sounds good to me!

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.

3 participants