Implement optional 'compact' and 'inline' item list settings, used for key map shortcuts

[aaronse]

Implemented optional 'compact' and 'inline' item list settings, used for key map shortcuts:

Before: 

After: 

[luc-github]

I did not use inline/compact because I got feedback people does not like compact controls especially on CNC

Additionnaly it may cause overload , unreadibility, more over with language like german which is very long

After several version of UI I tried to implement, I decided to make it simple and avoid complex implementation for simple things Sorry missed to translated key it is even more talktive

[aaronse]

imo - The warning messages should be on separate line and not cause the input text box to change width.

Am digging around code to see if/how that can be done... Sorry, am very new to this project and slow to contribute.

[luc-github]

yes but it is not the way inline is currently implemented , I did not spent tiime on this because was not used - and even without this message the label translation can be very long and so the shortcut lenght which just move the problem little bit forward

When testing need to test in worst condition not the best ^_^ this is Murphy Law and even ok because of Murphy Law

[aaronse]

This looks good to me, even in German...

Compared to existing behavior without edits in this pull request (+ putting validation on separate line) ...

[luc-github]

well the issue here are 1 - text overflow on X+ X- 2 - the modified shortcut is unreadable on Home X

Compared to readable version ? what is exactly the issue of readable version ? I only see the interline could be smaller that is all , no mention UI should be consistent with Macros List, Extra content List , well all UI actually

[aaronse]

Am editing code to put warning/error messages on separate line. I only meant for label ('key' or 'Schluessel') to be inlined with the text input field. Thoughts?

Sorry, I didn't understand:

1 - text overflow on X+ X-

[aaronse]

Thoughts on inline just label, but not warn/error message?

For example:

Let me know, I will update pull request.

[luc-github]

did you read my previous comment ?

[luc-github]

One possible mitigation would be not display Key: because only one entry currently just the control name then the input

[aaronse]

did you read my previous comment ?

yep

2 - the modified shortcut is unreadable on Home X

I don't see the issue? The Control+Shift+Alt+Backspace very unlikely edge case will renders ok on dekstop and tablet. And will even render ok on phone when they exit edit mode. This isn't an issue for phone users even, they probably won't be using (bluetooth) keyboard.

[aaronse]

One possible mitigation would be not display Key: because only one entry currently just the control name then the input

Sounds good to me. Want me to try editing? Will add showLabel=false property to itemlist

[luc-github]

no need - just add condition on label on

 let labelTxt
if (idList == "keymap" && type == "text") {
                                    labelTxt = ""
                                } else {
                                    labelTxt = label
                                }

                                return (
                                    <Field
                                        id={item.id}
                                        label={T(labelTxt)}

if only used for this case not need to add new flag - limite code is better - add flag only if more than 1 case

[luc-github]

even better is for idList == "keymap" is get the id of control as label and do not show the standalone label entry so interline would be limited

[luc-github]

I am out today , I will have a look in evening

[aaronse]

I am out today , I will have a look in evening

Sure thing, am US West Coast timezone. Cheers!

[luc-github]

this commit make the trick and I have also removed the unnecessary change now in formatItem, making code back to generic

https://github.com/luc-github/ESP3D-WEBUI/commit/cbd31127b3541355e5b7207c1c91ec492c8339ff

[aaronse]

Pulled, run dev-printer-marlin and tried out. Looks good to me :-)

[aaronse]

Thoughts on enabling keyboard users to always have "Use keyboard" enabled by default? When resetting CNC, User often needs to refresh webpage, having keyboard enabled (if configured in settings) by default helps.

"Use Keyboard" in Dashboard page's Panel List drop down still behaves same way, but initial default value on page load is based on preferences setting.

[luc-github]

this can be a setting in setting page / same for auto repeat, yes

[aaronse]

Pushed https://github.com/luc-github/ESP3D-WEBUI/pull/289/commits/d8ba542dfaaaee0a9fa3f5903a823963d48b3852 adding this setting.

[luc-github]

I am not sure it should be enabled by default

[luc-github]

in opposite way - should auto repeat be also part of dashboard menu ?

[aaronse]

no the way you did is always reset state to setting when going to dashboard I am not sure it should be enabled by default

Not sure I understand. Hope info below helps clarify my thinking/understanding...

Use Keyboard general setting is false by default when User first installs. IF User changes Use Keyboard general setting to true, then, dashboard Use Keyboard will be true when the User loads or refreshes the webpage. This is my intent.

User can still disable Use Keyboard from the dashboard. I didn't remove Use Keyboard panel lists dropdown from Dashboard page because I'm guessing you still want it there for reasons. I don't know why User still needs to enable and disable Use Keyboard from the Dashboard page now that code is fixed to not process key shortcuts if Terminal console (or other text input) fields has focus.

should auto repeat be also part of dashboard menu ?

The current behavior of Auto repeat general setting (not dashboard) makes sense to me.

[luc-github]

no the way you did is always reset state to setting when going to dashboard

I read to fast code change - and removed my comment

I am not sure it should be enabled by default

it was the answer about :

Thoughts on enabling keyboard users to always have "Use keyboard" enabled by default?

Let user enabling in settings is indeed ok

[luc-github]

Is compact still necessary ? it is not used actually

[aaronse]

Goal of compact: true is to not have empty line, which looks better to me. Your thoughts? It's being used and set true in "id": "keymap."

If "compact" : false or "compact" is not defined then setting renders like...

if "compact" : true then setting renders like...

Another example...

Before: 

After: 

Don't know why the text input width is less in the "After" version. I'll take a look at fixing the text input width issue if you think Compact is an improvement and should be merged to main.

[luc-github]

then should not be the default behaviour instead of add new flag (compact) ? is there any situation the compact as defaut behavior cause rendering issue ?

[aaronse]

I didn't (and still don't) know enough about settings rendering code to be able to assume this would improve rendering of all settings. That's why I only modified the new one, for now at least.

Let me know your preference?

[luc-github]

well if compact is better rendering - it is better to apply "display:table-cell" to itemEditor and check macro / extra panel instead - no ?

Note: the icon change flag too much on right is not related to that - it should be fixed in future when I have time

[luc-github]

so far looks ok for me

[aaronse]

Cool. Not clear what you want me to do?

[luc-github]

well as said remove compact flag from your PR and update itemEditor style accordingly to use the same behavior

[luc-github]

it should simplify your PR and so I can merge it

[aaronse]

Current code with just compact:true for keymap.... <div class="itemEditor" style={((compact) ? "display:table-cell;" : "")}>

Renders...

But, removing the setting and using display:table-cell for all results in Add Macro not rendering correctly,

<div class="itemEditor" style="display:table-cell;">

[aaronse]

Nevermind looks like the Add Macro render behavior is unrelated to Compact. So, am removing...

[luc-github]

it should not it should do same on my side it is due to the collabsed button text which is too long and should be trimmed somehow to avoid that

[luc-github]

please update here https://github.com/luc-github/ESP3D-WEBUI/blob/3.0/src/style/components/_control.scss#L84 not using style

[luc-github]

@aaronse I have fixed additionnal 2 small issues - bumped version and built packages - now ready to use I did not find proper way to handle the button oversized if text too long - text-overflow is not taken in account - and I have no time to spend on this now - so I have added a wrap for the buttons - but I have no fix if text itself is too long - so it display horizontal scrollbars ...