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

Dropdown ID inputs for trigger modal #1935

Open
wants to merge 8 commits into
base: dev/29-sodalite
Choose a base branch
from

Conversation

jpeterson976
Copy link
Contributor

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.

Screen Shot 2021-11-23 at 12 03 01 PM

@FrenjaminBanklin
Copy link
Contributor

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.

@FrenjaminBanklin
Copy link
Contributor

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.

@jpeterson976
Copy link
Contributor Author

I updated the way that this works for nav items a bit. For the focus:component actions it shows the actual information for the nodes on that page now, and if adding the action to the module itself it'll show the nodes for whatever the starting page is set to, even if it's not the current page.

@maufcost
Copy link
Contributor

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:
Uncaught TypeError: Cannot read properties of undefined (reading 'children') on editor-nav.js

Copy link
Contributor

@maufcost maufcost left a comment

Choose a reason for hiding this comment

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

Great job so far!

I just ran into a quick issue:

obo

I created a bunch of nodes, but the trigger options didn't update. It still has the old nodes before I added the new ones.

@jpeterson976
Copy link
Contributor Author

While testing the issue @maufcost found, I discovered another bug that causes a white screen when trying to add a focus:component action to a node when a new node has just been made. I managed to fix that, but the issue of the list not updating with new nodes for nav items is still there for now.

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.

@jpeterson976
Copy link
Contributor Author

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.

maufcost
maufcost previously approved these changes Mar 8, 2022
Copy link
Contributor

@maufcost maufcost left a 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.

Copy link
Contributor

@FrenjaminBanklin FrenjaminBanklin left a 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.

@jpeterson976
Copy link
Contributor Author

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.

@maufcost
Copy link
Contributor

maufcost commented Mar 15, 2022

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:
Screen Shot 2022-03-15 at 15 32 02

Just for reference, I am also getting some errors on the server side as well:
Screen Shot 2022-03-15 at 15 37 34

@jpeterson976
Copy link
Contributor Author

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.

@jpeterson976
Copy link
Contributor Author

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.

maufcost
maufcost previously approved these changes Mar 29, 2022
Copy link
Contributor

@maufcost maufcost left a 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.

@FrenjaminBanklin FrenjaminBanklin changed the base branch from dev/26-neon-apatite to dev/29-sodalite August 26, 2022 20:17
@FrenjaminBanklin FrenjaminBanklin dismissed maufcost’s stale review August 26, 2022 20:17

The base branch was changed.

@stale
Copy link

stale bot commented Jun 18, 2023

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.

@stale stale bot added the stale label Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants