-
Notifications
You must be signed in to change notification settings - Fork 584
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
Update Import Flow to Match Designs #9891
Merged
Merged
Changes from 27 commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
288ecb2
File import in progress...
thsparks 5ff28f2
Merge branch 'master' of https://github.com/microsoft/pxt into thspar…
thsparks 58b4c74
Fix build
thsparks f432c13
Fix setting rubric when no rubric is already set, and switch tabs aut…
thsparks 9565821
Scope common-modal-body styling to the import rubric modal
thsparks f0afd1c
Consolidate calls to confirm and replace the rubric
thsparks 8654a98
Add non-functional browse button and fix mouse event handling.
thsparks ca9e2e0
Working browse button, still needs a11y test
thsparks 22a989d
Merge branch 'master' of https://github.com/microsoft/pxt into thspar…
thsparks e803427
Cleanup
thsparks 5e7bcf3
Confirmation modal
thsparks 2db89f6
Error layout improvements and prettier
thsparks 45fc68f
Fade error in and out. Toast doesn't work because it's hidden behind …
thsparks 9c777bd
Screen reader support for error message.
thsparks 0185f72
More contrast for a11y
thsparks f486a3a
Themepack var
thsparks e69e2d8
Remove unnecessary null coalescing
thsparks b1f7f28
Clarify comment
thsparks 2bfe845
Add calls to stop propagation of drag events
thsparks 72d3161
Merge branch 'master' of https://github.com/microsoft/pxt into thspar…
thsparks 57de3e4
Move ConfirmationModalProps to types
thsparks b121b77
Rename ConfirmationModalProps to ConfirmationModalOptions
thsparks 43e83fd
Validate rubric criteria exists before trying to iterate over it (thi…
thsparks d714738
Use toast accent color for confirmation modal button
thsparks cd5dbcb
Remove small screen adjustments
thsparks f080ac4
Add await when loading rubric
thsparks 720693f
More renaming
thsparks e212501
Remove red from continue button
thsparks cbe0083
Prettier
thsparks File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
import { useContext, useEffect, useState } from "react"; | ||
import { AppStateContext } from "../state/appStateContext"; | ||
import { Modal } from "react-common/components/controls/Modal"; | ||
import { hideModal } from "../transforms/hideModal"; | ||
import { classList } from "react-common/components/util"; | ||
import css from "./styling/ConfirmationModal.module.scss"; | ||
|
||
export interface IProps {} | ||
export const ConfirmationModal: React.FC<IProps> = () => { | ||
const { state: teacherTool } = useContext(AppStateContext); | ||
|
||
function handleCancel() { | ||
hideModal(); | ||
teacherTool.confirmationOptions?.onCancel?.(); | ||
} | ||
|
||
function handleContinue() { | ||
hideModal(); | ||
teacherTool.confirmationOptions?.onContinue?.(); | ||
} | ||
|
||
const actions = [ | ||
{ | ||
label: lf("Cancel"), | ||
className: "secondary", | ||
onClick: handleCancel, | ||
}, | ||
{ | ||
label: lf("Continue"), | ||
className: classList( | ||
"primary", | ||
teacherTool.confirmationOptions?.cautionLevel === "high" ? css["caution"] : undefined | ||
), | ||
onClick: handleContinue, | ||
}, | ||
]; | ||
|
||
return teacherTool.modal === "confirmation" && teacherTool.confirmationOptions ? ( | ||
<Modal | ||
title={teacherTool.confirmationOptions.title} | ||
onClose={handleCancel} | ||
actions={actions} | ||
className={css["confirmation-modal"]} | ||
> | ||
{teacherTool.confirmationOptions.message} | ||
</Modal> | ||
) : null; | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
import { classList } from "react-common/components/util"; | ||
import { Strings } from "../constants"; | ||
import { NoticeLabel } from "./NoticeLabel"; | ||
import { useRef, useState } from "react"; | ||
import css from "./styling/DragAndDropFileSurface.module.scss"; | ||
import { Button } from "react-common/components/controls/Button"; | ||
|
||
export interface DragAndDropFileSurfaceProps { | ||
onFileDroppedAsync: (file: File) => void; | ||
errorMessage?: string; | ||
} | ||
export const DragAndDropFileSurface: React.FC<DragAndDropFileSurfaceProps> = ({ onFileDroppedAsync, errorMessage }) => { | ||
const [fileIsOverSurface, setFileIsOverSurface] = useState(false); | ||
const [errorKey, setErrorKey] = useState(0); | ||
const fileInputRef = useRef<HTMLInputElement>(null); | ||
|
||
function handleDragOver(event: React.DragEvent<HTMLDivElement>) { | ||
// Stop the browser from intercepting the file. | ||
event.stopPropagation(); | ||
event.preventDefault(); | ||
} | ||
|
||
function handleDragEnter(event: React.DragEvent<HTMLDivElement>) { | ||
event.stopPropagation(); | ||
event.preventDefault(); | ||
setFileIsOverSurface(true); | ||
} | ||
|
||
function handleDragLeave(event: React.DragEvent<HTMLDivElement>) { | ||
event.stopPropagation(); | ||
event.preventDefault(); | ||
setFileIsOverSurface(false); | ||
} | ||
|
||
function handleDrop(event: React.DragEvent<HTMLDivElement>) { | ||
event.stopPropagation(); | ||
event.preventDefault(); | ||
|
||
setFileIsOverSurface(false); | ||
|
||
const file = event.dataTransfer.files[0]; | ||
if (file) { | ||
processNewFile(file); | ||
} | ||
} | ||
|
||
function handleFileFromBrowse(event: React.ChangeEvent<HTMLInputElement>) { | ||
const file = event.target.files?.[0]; | ||
if (file) { | ||
processNewFile(file); | ||
} | ||
} | ||
|
||
function processNewFile(file: File) { | ||
// Change errorKey so that the error component resets (notably, resetting animations). | ||
setErrorKey(errorKey + 1); | ||
onFileDroppedAsync(file); | ||
} | ||
|
||
/* | ||
We can't use the drag-and-drop-file-surface directly to handle most drop events, because the child elements interfere with them. | ||
To solve this, we add a transparent div (droppable-surface) over everything and use that for most drag-related event handling. | ||
However, we don't want the transparent droppable-surface to intercept pointer events when there is no drag occurring, so | ||
we still use the drag-and-drop-file-surface to detect dragEnter events and only intercept pointer events on the droppable-surface | ||
after that has happened. | ||
*/ | ||
return ( | ||
<div className={css["drag-and-drop-file-surface"]} onDragEnter={handleDragEnter}> | ||
<div className={css["instruction-container"]}> | ||
<i className={classList("fas fa-file-upload", css["upload-icon"])}></i> | ||
<div className="no-select">{fileIsOverSurface ? Strings.ReleaseToUpload : Strings.DragAndDrop}</div> | ||
srietkerk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<div className={css["or-browse-container"]}> | ||
<span className={css["or-container"]}>{lf("or")}</span> | ||
|
||
{/* The button triggers a hidden file input to open the file browser */} | ||
<Button | ||
className={classList("link-button", css["browse-button"])} | ||
title={Strings.Browse} | ||
onClick={() => fileInputRef?.current?.click()} | ||
> | ||
{Strings.Browse} | ||
</Button> | ||
<input | ||
type="file" | ||
ref={fileInputRef} | ||
className="hidden" | ||
onChange={handleFileFromBrowse} | ||
aria-label={Strings.SelectRubricFile} | ||
accept=".json" | ||
/> | ||
</div> | ||
</div> | ||
|
||
{errorMessage && ( | ||
<div className={css["error-label-container"]} key={errorKey} role="alert" title={errorMessage}> | ||
<NoticeLabel severity="error">{errorMessage}</NoticeLabel> | ||
</div> | ||
)} | ||
|
||
<div | ||
className={classList(css["droppable-surface"], fileIsOverSurface ? css["dragging"] : undefined)} | ||
onDrop={handleDrop} | ||
onDragLeave={handleDragLeave} | ||
onDragOver={handleDragOver} | ||
/> | ||
</div> | ||
); | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 changes: 5 additions & 0 deletions
5
teachertool/src/components/styling/ConfirmationModal.module.scss
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
.confirmation-modal { | ||
button.caution { | ||
background-color: var(--pxt-toast-accent-error) | ||
} | ||
} |
73 changes: 73 additions & 0 deletions
73
teachertool/src/components/styling/DragAndDropFileSurface.module.scss
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
.drag-and-drop-file-surface { | ||
display: flex; | ||
flex-direction: column; | ||
align-items: center; | ||
justify-content: center; | ||
position: relative; | ||
|
||
background-color: var(--pxt-content-background); | ||
border-radius: 0.5rem; | ||
border: 1px dashed var(--pxt-content-secondary-foreground); | ||
|
||
padding: 2rem; | ||
min-height: 30vh; | ||
|
||
.instruction-container { | ||
display: flex; | ||
flex-direction: column; | ||
align-items: center; | ||
|
||
color: var(--pxt-content-secondary-foreground); | ||
font-size: 2rem; | ||
font-weight: 600; | ||
|
||
.upload-icon { | ||
font-size: 3rem; | ||
} | ||
|
||
.or-browse-container { | ||
display: flex; | ||
flex-direction: row; | ||
gap: 0.5rem; | ||
|
||
.or-container { | ||
font-weight: 400; | ||
} | ||
|
||
.browse-button { | ||
font-weight: 600; | ||
font-size: 2rem; | ||
} | ||
} | ||
} | ||
|
||
@keyframes fadeInAndOut { | ||
0% { opacity: 0; } | ||
5% { opacity: 1; } | ||
50% { opacity: 1; } | ||
100% { opacity: 0; } | ||
} | ||
|
||
.error-label-container { | ||
position: absolute; | ||
bottom: 1rem; | ||
animation: fadeInAndOut 4s forwards; | ||
|
||
i { | ||
display: none; | ||
} | ||
} | ||
|
||
.droppable-surface { | ||
position: absolute; | ||
top: 0; | ||
left: 0; | ||
right: 0; | ||
bottom: 0; | ||
|
||
pointer-events: none; | ||
&.dragging { | ||
pointer-events: all; | ||
} | ||
} | ||
} |
28 changes: 17 additions & 11 deletions
28
teachertool/src/components/styling/ImportRubricModal.module.scss
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,19 @@ | ||
.import-rubric { | ||
display: flex; | ||
flex-direction: column; | ||
gap: 0.5rem; | ||
.import-rubric-modal { | ||
div[class*="common-modal-body"] { | ||
background-color: var(--pxt-content-background); | ||
} | ||
|
||
.import-rubric { | ||
display: flex; | ||
flex-direction: column; | ||
gap: 0.5rem; | ||
|
||
.rubric-preview-container { | ||
max-height: 50vh; | ||
overflow-y: auto; | ||
border: 2px solid var(--pxt-content-foreground); | ||
border-radius: 0.3rem; | ||
background-color: var(--pxt-content-background-glass); | ||
.rubric-preview-container { | ||
max-height: 50vh; | ||
overflow-y: auto; | ||
border: 2px solid var(--pxt-content-foreground); | ||
border-radius: 0.3rem; | ||
background-color: var(--pxt-content-background-glass); | ||
} | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.