-
Notifications
You must be signed in to change notification settings - Fork 34
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
Dropdown ID inputs for trigger modal #1935
base: dev/29-sodalite
Are you sure you want to change the base?
Dropdown ID inputs for trigger modal #1935
Conversation
There are some stability issues being introduced here. It seems to be okay for adding triggers to certain chunks in the editor, but not certain others. For example: create a new button node, open the triggers menu for that button, then try adding either a new trigger or a new action. Doing either will completely break the editor. Trying to add triggers to a page will break the editor in the same way. |
Fewer breaks, but it looks like buttons are still behaving weirdly. Create a new button node, then change the 'Then' for any action to 'Focus on a specific item'. Same breakage as before. The behavior is a little weird with trying to add 'Focus on a specific item' triggers to pages or to the module itself. Options in the drop-down all show as 'hidden'. I'm guessing they correspond to the available pages, but it's unclear - and also buggy in the viewer, though I suspect that's always been the case. Not sure what to do there - either disable the 'Focus on a specific item' actions for page triggers or just let it ride for now. |
I updated the way that this works for nav items a bit. For the |
I didn't get to actually test your changes, but Obojobo breaks when you create a new module and access it. I can only access the modules that I already had locally. New ones break. The error stack starts at: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While testing the issue @maufcost found, I discovered another bug that causes a white screen when trying to add a It seems that the reason it's not updating is because the nav just doesn't get re-rendered until the more info box closes, so it doesn't receive the updated list of elements until then. I'm looking into how to get around that now. |
Should be ready for review now. With the way things are passed around it took me a while to figure it out, but the trigger modals in the nav should be properly updating with new nodes. I also got it to work with the triggers for the module itself. It'll show the elements for whatever the start page is, even if it's not the currently active page. But for some reason the model with all the information doesn't update with new pages until a refresh, so if the start page is set to a newly created page it will just revert back to the text ID input. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working pretty well! Great job, @jpeterson976 :) The triggers are all applied correctly as far as my testing goes and the nodes are updated according to the current selected page.
The dropdowns also become regular text inputs when one changes the ids manually on the XML and JSON editors, which will make it possible for me to add validation to those fields after this has been merged in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New action triggers default to nav:goto
, which is fine, but the default action trigger appears to be incompatible with the optional dropdown because it lacks an 'id' property.
This should be a pretty quick and easy fix. You've already mentioned the issues when setting up focus:component
triggers on the module as a whole with regards to changing the starting page, so we could potentially let it ride - but as it is, it's pretty easy for authors to get themselves into a situation where their content doesn't work and they're not sure why. If possible we might need to add some logic on the viewer side to prevent crashes in the event of a focus:component
event trying to target an item that's not on the current page. That could also be its own issue, maybe.
It was in fact a quick and easy fix. Good catch, should be good to go now. I think that adding logic to account for targeting a component not on the current page is definitely a good idea, though it's likely its own whole can of worms so making it into its own issue seems like the way to go to me. The dropdowns should help prevent that from occurring to some degree, at least. |
Just like before: amazing work with this, @jpeterson976 ! Now, I am not sure if that was already there before this issue, but I created a new page, added a bunch of nodes to it, and created a trigger on the new page that's supposed to scroll the user to a specific node in the page. However, this action is not working for me on new pages. Only on the first one. Can anyone replicate this? Here's the trigger configuration I used for the new page: Just for reference, I am also getting some errors on the server side as well: |
I'm getting the same behavior both on this branch and on dev/28, where it only actually scrolls on the first page. Not really sure what's causing that, but it seems like it's pre-existing. I haven't been able to replicate that server error myself, but I'll do a little more playing around with it tomorrow to try and figure out what's up. |
With some additional testing, I can't seem to recreate the server error on my end. So I'm not sure what's going on there or if there's actually an issue with the code here that's potentially causing it. The not scrolling on anything other than the first page is definitely a thing, but since it doesn't seem to be related to the changes I've made here I think it can be turned into a new issue on its own. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM then! I didn't find any more bugs on my part. Awesome work.
The base branch was changed.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep our backlog under control, but we thank you for your contributions. |
Closes #1622
Triggers modal now has dropdowns for actions that take in an id. For ones that go to a page, it'll show the page title similar to selecting the start page for a module, and for
focus:component
it'll give the type of chunk and grab a preview of the text for that chunk if applicable. It'll also revert back to a normal input box if the id that's already there is somehow invalid, and automatically swap back to the dropdown once the entry is either valid or empty.There were a few node types where getting a text preview was a little less obvious. For math equations it just shows the raw latex, lists will show text from only the first bullet point, and for tables it will only look at text in the top left cell. If anyone has any potentially better ideas I'm definitely open to changing how it works for these, or changing what's shown in the labels altogether if need be.