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

Move user-selectable options into addInputBlock() #50

Closed
wants to merge 1 commit into from

Conversation

RootPrivileges
Copy link
Contributor

Move dropdowns out of addActionsBlock(), where they don't seem to affect
the state, and so polls are not created with correct settings.

Resolves #49

This will conflict with #40, but the changes to the same file in there should be easy to migrate into the addActionsBlock() in my code as an additional element in the array.

Move dropdowns out of addActionsBlock(), where they don't seem to affect
the state, and so polls are not created with correct settings.

Resolves sampaiodiego#49
@sampaiodiego
Copy link
Owner

that's awesome @RootPrivileges , thank you for all the investigation you have made to fix this..

this is an issue with Rocket.Chat, and it will be fixed in the upcoming 3.6.0 later this month.. but would be awesome to have it fixed earlier on the app itself.

When testing your PR I found an issue though, when I hit "Add choices" it loses the option I chose on multiple/single choices and shows "Open vote" instead.. not sure it is another issue with rocket.chat or something in the app.

@RootPrivileges
Copy link
Contributor Author

If it's an upstream bug that is going to be fixed soon, not sure if merging is the right idea. This was my first dive into Rocket.Chat apps and I wasn't sure of the correct behaviour of the various APIs.

It seems most logical to me that user inputs should be held inside an addInputBlock() and would defend against a future regression; but if this isn't preferred, then feel free to reject, pending 3.6.0 being released.

@RootPrivileges
Copy link
Contributor Author

RootPrivileges commented Aug 18, 2020

Interesting on the bug where it forgets the option - I'm able to reproduce that too. Sorry, I hadn't noticed it was doing that when I made my PR!

I'm inclined to say that may also be a Rocket.Chat issue - I've not made any substantial changes to the code beyond splitting into separate addInputBlock() chunks. The issue manifests even if I use a different blockId in each section.

If I remove the placeholder variable from newStaticSelectElement({actionId: 'visibility'}), it instead shows "select a option" (lack of capitalisation and wrong use of "a/an" are from the message) but that string isn't anywhere in this repo, so it must be retrieving the default placeholder from somewhere in another codebase? This occurs even if the confidentiality option has been modified before adding a new option.

@RootPrivileges
Copy link
Contributor Author

RootPrivileges commented Aug 18, 2020

I'm reasonably sure now this is a bug somewhere upstream. To test, I added the following new addInputBlock() after the previous two:

    block.addInputBlock({
        blockId: 'config',
        element: block.newStaticSelectElement({
            placeholder: block.newPlainTextObject('Test field'),
            actionId: 'demo',
            initialValue: 'test',
            options: [
                {
                    text: block.newPlainTextObject('Test field'),
                    value: 'test',
                },
                {
                    text: block.newPlainTextObject('Working field'),
                    value: 'working',
                },
            ],
        }),
        label: block.newPlainTextObject(''),
    });

Pressing the "Add a choice button" here causes the placeholder for the input below to be moved up one in both cases:

Screenshot from 2020-08-18 19-49-42

Screenshot from 2020-08-18 19-49-58

@RootPrivileges
Copy link
Contributor Author

I think I'm going to have to leave this here. For a quick fix, moving both addInputBlock() defintions above the for loop prevents the bug, with the obvious side effect of moving the options to the top of the modal.

It's definitely something to do with dynamically modifying the number of options and the for block rendering, as setting options = 3 on line 7 causes the modal to be displayed correctly on first open. But I don't know enough to say where the bug is happening outside of this app.

@sampaiodiego
Copy link
Owner

thanks again @RootPrivileges for your awesome work on investigating this issues.. the fix for "single choice" select was just published on rocket.chat 3.5.3... I'll talk to the Rocket.Chat Apps team regarding these other weird behaviors you found..

@sampaiodiego
Copy link
Owner

@RootPrivileges I have test this again and everything seems fixed in current rocket.chat version, so I'll be closing this PR and move on to fix other issues, where I'll move files around, ok?

I really appreciated all your efforts

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.

Single choice option not working
2 participants