sampaiodiego / rocket.chat.app-poll

Rocket.Chat App for creating polls.
MIT License
50 stars 39 forks source link

Move user-selectable options into addInputBlock() #50

Closed RootPrivileges closed 3 years ago

RootPrivileges commented 4 years ago

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/rocket.chat.app-poll#49

This will conflict with sampaiodiego/rocket.chat.app-poll#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.

sampaiodiego commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

@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