Update Import Flow to Match Designs #9891

thsparks · 2024-02-28T00:33:14Z

This updates our import flow (and confirmation dialog) to match our v1 designs.

I tried reusing https://github.com/microsoft/pxt/blob/master/webapp/src/draganddrop.ts, by moving it into browserutils and adding extra events for when drag enters and exits/leaves, but it didn't quite work out. The enter and exit events caused a lot of flickering when hovering over child elements, which I believe would require a workaround somewhat like the one I've described in the DragAndDropFileSurface in this PR. I wasn't comfortable adding that workaround at such a global scale (and it isn't necessary when you don't care about enter/exit), so I decided not to reuse.

Upload target: ~~https://makecode.microbit.org/app/5fc5d7fc1489c936a8bdc40422b6a5b3a5926855-6b68fdda25--eval~~ https://makecode.microbit.org/app/3743a0ed15ed73ae921ee8bf75533603348b2665-b7ed2b5339--eval

Screenshots

Import:

File hover:

Confirm:

Invalid file:

Small screen:

…ks/teachertool/rework_import

…omatically to rubric

…ks/teachertool/rework_import

…the faded background of the modal.

thsparks · 2024-02-28T00:33:51Z

Color contrast passes a11y check:

srietkerk · 2024-02-28T17:44:34Z

teachertool/src/components/ConfirmationModal.tsx

+import { CautionLevel } from "../types";
+import css from "./styling/ConfirmationModal.module.scss";
+
+export type ConfirmationModalProps = {


Should this type be somewhere in the types folder?

It could be. I thought, since it was so closely tied to the component, it made sense to keep here, but I can move it to types.

Yeah, I think it should be moved to stay consistent.

srietkerk · 2024-02-28T17:46:54Z

teachertool/src/components/ConfirmationModal.tsx

+
+    return teacherTool.modal === "confirmation" && teacherTool.confirmationProps ? (
+        <Modal
+            title={teacherTool.confirmationProps.title ?? ""}


There shouldn't be a case where the title is falsey, right? The title is required in confirmationProps, so it this check shouldn't result in an empty string since we already check that confirmationProps is populated above?

srietkerk · 2024-02-28T17:57:12Z

teachertool/src/components/DragAndDropFileSurface.tsx

+
+        setFileIsOverSurface(false);
+
+        const file = event.dataTransfer.files[0];


I think this can be done in a follow-up PR, but I think we need some handling for uploading multiple files. When I dragged in 3 JSON files, it's unclear to me which JSON file is going to get used or even if my other rubric files will get stored or something. If we don't want to support the multiple file scenario, I think just have an error or warning saying that we can only accept one rubric file at a time will helpful.

This will need to be done for the browse scenario as well.

Yeah, I'm not sure what uploading multiple files would mean in terms of import, so my inclination would be to disallow it and provide an error message, but we can discuss as a team and follow up in a separate change.

Just importing the first file is fine. I'd prefer not to treat this as an error or warning. The fewer alarm states we create, the better. If we can make it "just work", that's good. The user will learn how it behaves through observation.

I think this might just be something we want to cover/ask in user testing. My thoughts are that because we have the modal where we tell the user that importing will override their current rubric, especially with the red coloring, we are putting the user in an alarm state, so I think we should give them the most piece of mind that we possibly can. Personally, when I saw the modal, I didn't even read the rubric title in the top bar because most of my focus was on the button and the details outlining what importing is going to do, so, when I dragged multiple rubrics in, I got nervous not knowing which rubric was going to override my current rubric. In my mind, a "just work" scenario if I drag multiple files in would be that all of my rubrics are now on the site and accessible. I know this isn't what we're going for right now, but I still think it's a valid scenario to consider. For now, I think we can take the first file, but I think we should make an issue for supporting uploading multiple rubrics at once.

On that note, I don't think we should use a red/alarm color for the Continue button. The primary button color should be sufficient.

srietkerk · 2024-02-28T17:59:37Z

teachertool/src/components/DragAndDropFileSurface.tsx

+}
+export const DragAndDropFileSurface: React.FC<DragAndDropFileSurfaceProps> = ({ onFileDroppedAsync, errorMessage }) => {
+    const [fileIsOverSurface, setFileIsOverSurface] = useState(false);
+    const [errorKey, setErrorKey] = useState(0); // Used to reset animations on the error label.


Even with the comment here, I'm not sure I understand what errorKey does.

By changing the key of the component, we force react to unmount the component and mount a new one, which resets the animation state. If we don't do this, then the animation only runs on the first error and no subsequent ones, since from the component's perspective, the animation has already run.

Ohh, gotcha. Makes sense, so errorKey could technically be any type, as long as we can make a quick change to it to cause an unmount and mount, and that's why using numbers is convenient?

As long as it's a valid key, then yes (though I think that's really only strings and numbers).

teachertool/src/components/DragAndDropFileSurface.tsx

…ks/teachertool/rework_import

thsparks · 2024-02-28T18:54:09Z

theme/themepacks.less

@@ -25,6 +25,7 @@
    --pxt-content-background-glass: #C7D2FE40;
    --pxt-content-foreground: #1E293B;
    --pxt-content-accent: #EEF2FF;
+    --pxt-content-secondary-foreground: #666666;


@eanders-ms I feel like I'm adding colors here kind of ad-hoc and I'm probably already making a mess of things. Did you have any conventions or systems in mind for how these should be organized and named? If not, maybe we could chat and come up with something (reasonably) consistent.

I considered glass/smoke/accent/etc but none of those quite seemed to fit for this one, IMO.

This needs more design thinking, which will have to happen before we implement this for Arcade. A little bit of mess is fine for now, but I hope we can avoid adding too many ad-hoc values. Use an existing color variable if possible.

eanders-ms · 2024-02-28T19:31:59Z

I tried dropping tsconfig.json and got this error in the console:

eanders-ms · 2024-02-28T19:39:27Z

teachertool/src/types/index.ts

@@ -44,3 +46,11 @@ export type CarouselCardSet = {
 };

 export type RequestStatus = "init" | "loading" | "error" | "success";
+
+export type ConfirmationModalProps = {


The inclusion of Props in the name tells me this configures a React component. Props is uncommon for non-React configuration. I suggest using something else here. Suggestions:
ConfirmationModalOptions
ConfirmationModalParams

eanders-ms · 2024-02-28T19:47:00Z

teachertool/src/components/DragAndDropFileSurface.tsx

+
+        setFileIsOverSurface(false);
+
+        const file = event.dataTransfer.files[0];


Just importing the first file is fine. I'd prefer not to treat this as an error or warning. The fewer alarm states we create, the better. If we can make it "just work", that's good. The user will learn how it behaves through observation.

eanders-ms · 2024-02-28T20:00:32Z

teachertool/src/components/styling/ConfirmationModal.module.scss

@@ -0,0 +1,5 @@
+.confirmation-modal {
+    button.caution {
+        background-color: rgb(255, 71, 71);


Can this color be derived from the toast error color? I think it will be the same color for all themes, so not a big issue to appear as a literal here. Though high-contrast might be a special case.

I tried that at first, but the button looked disabled:

That said, the toast's accent color might work. I'll switch to that:

eanders-ms · 2024-02-28T20:01:53Z

teachertool/src/components/styling/DragAndDropFileSurface.module.scss

+        }
+    }
+
+    @media screen and (max-height: 720px) {


We should be using breakpoints from react-common, not creating our own.

Probably better to just remove this for now. We have an issue on the board tracking the need to implement responsive layout app-wide. Something we can address post v1.

eanders-ms · 2024-02-28T20:05:40Z

teachertool/src/transforms/loadRubricAsync.ts

-
-    setRubric(json);
-    setActiveTab("rubric");
+    replaceActiveRubricAsync(json);


Suggested change

replaceActiveRubricAsync(json);

await replaceActiveRubricAsync(json);

…s can be undefined if json parsing doesn't have the field)

thsparks · 2024-02-28T20:18:00Z

I tried dropping tsconfig.json and got this error in the console:

Good catch. Added a check in verifyRubricIntegrity to ensure criteria is not undefined.

eanders-ms · 2024-02-28T20:35:46Z

teachertool/src/state/actions.ts

@@ -55,6 +55,11 @@ type SetRubric = ActionBase & {
    rubric: Rubric;
 };

+type SetConfirmationProps = ActionBase & {


Can we change this name (and various related identifiers) to match?

Suggested change

type SetConfirmationProps = ActionBase & {

type SetConfirmationOptions = ActionBase & {

eanders-ms

Looks great!

srietkerk

Nice!

thsparks added 16 commits February 23, 2024 17:09

File import in progress...

288ecb2

Merge branch 'master' of https://github.com/microsoft/pxt into thspar…

5ff28f2

…ks/teachertool/rework_import

Fix build

58b4c74

Fix setting rubric when no rubric is already set, and switch tabs aut…

f432c13

…omatically to rubric

Scope common-modal-body styling to the import rubric modal

9565821

Consolidate calls to confirm and replace the rubric

f0afd1c

Add non-functional browse button and fix mouse event handling.

8654a98

Working browse button, still needs a11y test

ca9e2e0

Merge branch 'master' of https://github.com/microsoft/pxt into thspar…

22a989d

…ks/teachertool/rework_import

Cleanup

e803427

Confirmation modal

5e7bcf3

Error layout improvements and prettier

2db89f6

Fade error in and out. Toast doesn't work because it's hidden behind …

45fc68f

…the faded background of the modal.

Screen reader support for error message.

9c777bd

More contrast for a11y

0185f72

Themepack var

f486a3a

thsparks requested a review from a team February 28, 2024 00:33

thsparks mentioned this pull request Feb 28, 2024

Add Microbit Themepack Color for Teacher Tool Import Dialog microsoft/pxt-microbit#5512

Merged

srietkerk reviewed Feb 28, 2024

View reviewed changes

thsparks added 5 commits February 28, 2024 10:23

Remove unnecessary null coalescing

e69e2d8

Clarify comment

b1f7f28

Add calls to stop propagation of drag events

2bfe845

Merge branch 'master' of https://github.com/microsoft/pxt into thspar…

72d3161

…ks/teachertool/rework_import

Move ConfirmationModalProps to types

57de3e4

thsparks commented Feb 28, 2024

View reviewed changes

thsparks self-assigned this Feb 28, 2024

eanders-ms reviewed Feb 28, 2024

View reviewed changes

Rename ConfirmationModalProps to ConfirmationModalOptions

b121b77

eanders-ms reviewed Feb 28, 2024

View reviewed changes

thsparks added 4 commits February 28, 2024 12:09

Validate rubric criteria exists before trying to iterate over it (thi…

43e83fd

…s can be undefined if json parsing doesn't have the field)

Use toast accent color for confirmation modal button

d714738

Remove small screen adjustments

cd5dbcb

Add await when loading rubric

f080ac4

eanders-ms reviewed Feb 28, 2024

View reviewed changes

More renaming

720693f

eanders-ms approved these changes Feb 28, 2024

View reviewed changes

srietkerk approved these changes Feb 28, 2024

View reviewed changes

thsparks added 2 commits February 28, 2024 14:16

Remove red from continue button

e212501

Prettier

cbe0083

thsparks merged commit bb4abac into master Feb 28, 2024
6 checks passed

thsparks deleted the thsparks/teachertool/rework_import branch February 28, 2024 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Import Flow to Match Designs #9891

Update Import Flow to Match Designs #9891

thsparks commented Feb 28, 2024 •

edited

Loading

thsparks commented Feb 28, 2024

srietkerk Feb 28, 2024

thsparks Feb 28, 2024

srietkerk Feb 28, 2024

srietkerk Feb 28, 2024

srietkerk Feb 28, 2024

thsparks Feb 28, 2024

eanders-ms Feb 28, 2024

srietkerk Feb 28, 2024 •

edited

Loading

eanders-ms Feb 28, 2024

srietkerk Feb 28, 2024

thsparks Feb 28, 2024

srietkerk Feb 28, 2024

thsparks Feb 28, 2024

thsparks Feb 28, 2024

eanders-ms Feb 28, 2024

eanders-ms commented Feb 28, 2024

eanders-ms Feb 28, 2024

eanders-ms Feb 28, 2024

eanders-ms Feb 28, 2024

thsparks Feb 28, 2024

eanders-ms Feb 28, 2024

eanders-ms Feb 28, 2024

eanders-ms Feb 28, 2024

thsparks commented Feb 28, 2024

eanders-ms Feb 28, 2024

eanders-ms left a comment

srietkerk left a comment


		setFileIsOverSurface(false);

		const file = event.dataTransfer.files[0];

	replaceActiveRubricAsync(json);
	await replaceActiveRubricAsync(json);

	type SetConfirmationProps = ActionBase & {
	type SetConfirmationOptions = ActionBase & {

Update Import Flow to Match Designs #9891

Update Import Flow to Match Designs #9891

Conversation

thsparks commented Feb 28, 2024 • edited Loading

Screenshots

thsparks commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srietkerk Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eanders-ms commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thsparks commented Feb 28, 2024

Choose a reason for hiding this comment

eanders-ms left a comment

Choose a reason for hiding this comment

srietkerk left a comment

Choose a reason for hiding this comment

thsparks commented Feb 28, 2024 •

edited

Loading

srietkerk Feb 28, 2024 •

edited

Loading