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

Select dialog #19

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Select dialog #19

wants to merge 8 commits into from

Conversation

hrueger
Copy link

@hrueger hrueger commented Oct 25, 2019

added a select dialog

@coveralls
Copy link

coveralls commented Oct 25, 2019

Coverage Status

Coverage decreased (-8.3%) to 84.135% when pulling 78449ad on hrueger:select-dialog into a009d84 on coderaiser:master.

@hrueger
Copy link
Author

hrueger commented Oct 26, 2019

Unfortunately, I don't know how to write the tests...
Is it ok anyway?

@coderaiser
Copy link
Owner

That's a very good idea to make such a feature :). It absolutely feats to smalltak scope, and can be reused in Cloud Commander which have own implementation for User Menu, but would be better if it used smalltalk.

Would be amazing to join Cloud Commander's implementation of select with the one you provided, for this purpose would be great to add keys support and to use smalltalk here instead of @cloudcmd/modal.

@hrueger
Copy link
Author

hrueger commented Oct 27, 2019

I tried to implement the key events, but unfortunately it doesnt work with selectedIndex and the arrow events, as the (I assume every browser, only tested with the latest Firefox) browser navigates in a select element with the arrow keys automatically. In the demo of cloudcommander the arrow keys and j and k keys dont give the same result...
As the focus is already in the select element when the dialog is opened, I would leave the navigating with the arrow keys to the browser.

@hrueger
Copy link
Author

hrueger commented Oct 27, 2019

Also I have a question:
In my project AGLight I often need a dialog like this:
grafik
You can choose between two or more options and they are displayed as cards.

Can I add this to smalltalk? Or would you say that this is a bit too unusual?

@coderaiser
Copy link
Owner

@hrueger could you render template you are going to show and then just pass message as html markup?

@hrueger
Copy link
Author

hrueger commented Oct 28, 2019

Yes, of course I can. But I'll make a new pull request for this feature.

@coderaiser
Copy link
Owner

@hrueger good idea 🙂

@hrueger
Copy link
Author

hrueger commented Oct 31, 2019

I just added an option for multiple selects. Is there anything more which has to be done before this PR can be merged?

@hrueger
Copy link
Author

hrueger commented Nov 9, 2019

Any update on this?

@coderaiser
Copy link
Owner

Could you please get back key names?

const TAB = 9;
const event = {
    keyCode: TAB,
}

@hrueger
Copy link
Author

hrueger commented Nov 11, 2019

Unfortunately, this causes the linting test to fail: see line 227f.

@coderaiser
Copy link
Owner

Lint is fixed :), also PR missing tests. There is select in Cloud Commander's user menu, what do you think about porting it to smalltalk :)? It has a lot features implemented, and can significantly simplify code base of a file manager, and make smalltalk benefit from select.

@hrueger
Copy link
Author

hrueger commented Jan 7, 2020

Thanks for the linting fix and the tests. Unfortunately, I don't have time to port this to smalltalk.

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.

3 participants