netcreateorg / netcreate-itest

Developing the 2.0 version of NetCreate
https://github.com/netcreateorg/netcreate-2018
Other
0 stars 0 forks source link

Feature: Additional Comment Prop Types #201

Closed benloh closed 1 month ago

benloh commented 2 months ago

This adds new prompt types:

screenshot_1540

Dropdown

A standard browser dropdown menu. You can use emojis, e.g. 'πŸ˜€ I like it'

Checkbox

Select one or more checkbox items.

Radio

Select one and only one item. List displayed vertically in a column of radio buttons.

Likert

Select one and only one item. List displayed horizontally in a row as buttons.

You can use any text and any number of items for the likert items. And actually the order does not matter either. The selection is made by matching the text exactly. So you can use ["1", "2", "3"] or emojis ['πŸ’™', 'πŸ’š', 'πŸ’›', '🧑', '🩷']

Discrete Slider (aka "Star Rating")

This is the star rating. This is actually handled similar to the "Likert" but it'll display the selected item in a stack. Each item can use a different value. For example, you could actually use a different color for every star. e.g. if you use ['πŸ’™', 'πŸ’š', 'πŸ’›', '🧑', '🩷'] and select the third item, you'd see πŸ’™πŸ’šπŸ’›.

Technical Notes

To Do

benloh commented 2 months ago

@jdanish This is a rough first pass at the prompt types. To test:

  1. check out the dev-bl/types branch
  2. Create a new comment
  3. Select the "demo" comment type
  4. Try all the values.

You can customize the prompts in dc-comments:184 or thereabouts. Search for DEFAULT_CommentTypes.

Additional notes and comments later when I have more time.

jdanish commented 2 months ago

Running into an error on build even after trying npm ci

19:26:40 - error: Processing of app/view/netcreate/components/NCComment.jsx failed. Error: Could not load module './NCCommentPrompt' from '/Users/jdanish/gitcode/netcreate-itest/app/view/netcreate/components'. Make sure the file actually exists. Stack trace was suppressed. Run withLOGGY_STACKS=1to see the trace. 19:26:41 - info: compiled 1041 files into 5 files, copied 4 in 9.0 sec

benloh commented 2 months ago

@jdanish Sorry about that. Forgot to stage a new module. It should be up now.

jdanish commented 2 months ago

@jdanish Sorry about that. Forgot to stage a new module. It should be up now.

No worries. Confirming I can now build and will begin testing.

jdanish commented 2 months ago

Minor issue, but we should have some sort of display if nothing is selected ...

This is really cool, by the way.

Screenshot 2024-05-05 at 12 44 35 PM
jdanish commented 2 months ago

E.g. if a user selected and saved "Apple" in their checkbox, and then you change the template so that it read "Apples" instead, the user's checkbox would be unchecked. So this should mostly be a problem if you change templates mid-study.

Agreed. I assume that budget allowing we'd either a) just not worry about it, b) hand change any comments effected, or c) have the future template editor handle it gracefully (e.g., if we change the template from Apple to Apples it'll assume we need to convert data that reads Apple to Apples), but we can revisit as needed.

jdanish commented 2 months ago

Opps, z-index is still not perfect. Node displays are on top of the alert. I think we want (back to front): node display, alert, actual comments.

Screenshot 2024-05-05 at 12 51 26 PM

Also ...

Screenshot 2024-05-05 at 12 52 31 PM
benloh commented 2 months ago

Agreed. I assume that budget allowing we'd either a) just not worry about it, b) hand change any comments effected, or c) have the future template editor handle it gracefully (e.g., if we change the template from Apple to Apples it'll assume we need to convert data that reads Apple to Apples), but we can revisit as needed.

Actually if this is important we'd want to change how we store the data. I had gone back and forth between trying to decide if we want a simple match vs an indexed match vs indexed with text. I had decided that it would unnecessarily complicate the data structures and the readability of the comment data (we'd have to insert delimiters to keep the data simple, which makes them difficult to read), and sort order would also become a problem (e.g. if you changed the sort order of the items in the template, all of your data would be completely lost). It seemed simpler to rely on a text match.

jdanish commented 2 months ago

Actually if this is important we'd want to change how we store the data. I had gone back and forth between trying to decide if we want a simple match vs an indexed match vs indexed with text. I had decided that it would unnecessarily complicate the data structures and the readability of the comment data (we'd have to insert delimiters to keep the data simple, which makes them difficult to read), and sort order would also become a problem (e.g. if you changed the sort order of the items in the template, all of your data would be completely lost). It seemed simpler to rely on a text match.

I think your solution is preferred. This kind of change should be exceedingly rare so let's keep the readability priority.

jdanish commented 1 month ago

I'll keep tinkering / testing, but it is looking good to me, and I'd be inclined to merge once we remove the "placeholder" text?

benloh commented 1 month ago

@jdanish I think we're ready to merge this, but given all the changes it's probably worth a quick spin in case I missed something. Main visible changes should mostly be obvious from the commit messages after your last comment, but I can add a change list if you prefer.

jdanish commented 1 month ago

Oh, yes I've been testing with this, and we did a mega test last night to make 45 minutes worth of log files. Seems pretty stable! A few quirks but we couldn't reproduce them and reloading the screen fixed them. So I'd say let's push and I'll keep an eye out if we can reproduce those but then just log new issues since this feels pretty stable.

benloh commented 1 month ago

Cool, thanks!