Dashboard fixes and refactors #3614

jcushman · 2024-10-06T20:14:15Z

Changes made while responding to Becky's QA with developer-playground = true:

With the waffle flag developer_playground = false, submit an invalid URL, so that you are offered the chance to upload your own file. Click the button. Nothing happens: I think this feature has not yet been fully implemented in Vue. Set the flag true. Repeat. A modal DOES appear. But, it is clearly buggy/unfinished. (Also, like the Vue link batch modal, it is weirdly skinny.)

New upload modal implemented.

Select an empty folder. Observe that the link list overflows and scrolls horizontally.
Log out, then log in again as an admin, org, or registrar user. No folder will be selected. Observe that the UI reports, "This is an empty folder". Repeat in prod or with the flag off, observe that it doesn't.

Fixed up status messages for folders:

Simpler HTML if showing status message, to avoid scrollbars.
"No folder selected." if no folder selected.
Spinner while loading, instead of unstyled "Loading" text.

In prod, select a folder with links. Click into the link list's search box. Press tab until keyboard focus is on the caret button that will expand the first link. Press enter to expand the link. Observe that keyboard focus remains on the button. Press it again to close, and observe that focus remains again. Repeat the process with this branch; observe that both times, keyboard focus is dropped. (Your browser might help you out and set it somewhere close by; browser behavior here varies.)

If using keyboard, focus stays on caret button when opening/closing. If using mouse to click the rest of the row, focus goes to first input box on open, and does nothing on close.

The label for the caret button that toggles a link's details include "{{ link.guid }}", verbatim, instead of that variable's value.

Fixed.

Other changes in this PR:

Refactored spinner component that is easier to use in layouts.
Refactored existing form components made for WIP upload form.
Cleaned up capture button styles -- removed focus outline while capturing, fixed up padding, progress bar replaces spinner.
Some globalStore cleanup -- moved some values from store to props, used a more consistent style for storing references to components.
Improved the fetchDataOrError fetch wrapper, and used consistently in place of fetch() and one-off error handling.
Fixed jstree drag and drop style -- turns out this was messed up because of having body { height: 100% } for a sticky footer, meaning our window never really scrolled. Added a drag and drop indicator with a little more style.

I did my best to break these all out into separate commits, but probably mis-organized a line or two.

- takes up space in the DOM, easier to style - horizontally centered - uses single config object for spin.js params - default aria attributes

- fetchDataOrError accepts data option that can be either formData or json data - fetchDataOrError returns response data, if any, on error, and provides access to response object

body { height: 100% } causes event.pageX and event.pageY not to work, because body scrolls rather than window scrolling

- show spinner instead of unstyled text while loading - show "No folder selected" instead of empty div when no folder selected

- captureGUID and captureErrorMessage as props instead of global state - CreateLink uses fetch wrapper

codecov · 2024-10-06T20:24:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.25%. Comparing base (c72853c) to head (2abbcda).
Report is 6 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3614      +/-   ##
===========================================
+ Coverage    69.19%   69.25%   +0.05%     
===========================================
  Files           53       54       +1     
  Lines         7193     7207      +14     
===========================================
+ Hits          4977     4991      +14     
  Misses        2216     2216

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rebeccacremona

Whew! This is a lot. I gave it all a read-through, but did not pull down to test. I had one question about something possibly missed during refactoring, and then just made a couple remarks. Since this is behind the feature flag, let's QA in stage 🙂

Thanks for taking on this project!

rebeccacremona · 2024-10-07T16:14:03Z

perma_web/frontend/components/UploadForm.vue

- for (const [key, { value }] of Object.entries(formData.value)) {
- formDataObj.append(key, value);
+ formDataObj.append('file', file.value);
+ if (!props.captureGUID) {


Just noting that the non-Vue version of this form actually doesn't do the patch version of this: it always makes a new Perma Link.

So, we'll have to see if no title/description/url, ends up being what users want here. Could end up that they prefer the current behavior.

Yeah huh, I didn't actually trigger that version in my testing, just did captures of invalid domains. How does one show it?

I think the quickest way is, you could throw a 1/0 or raise HaltCaptureException someplace in https://github.com/harvard-lil/perma/blob/develop/perma_web/perma/celery_tasks.py#L312, to make a failed capture.

rebeccacremona · 2024-10-07T16:17:05Z

perma_web/frontend/components/UploadForm.vue

+ if (error) {
+ console.log(error, response)
+ if (data) {
+ errors.value = data


Does this want to be globalErrors.value, like below, too?

Theoretically no -- the idea here is the API would return 400 with data {title: "Terrible title"} and it would render the same way as the validation section previously in the function. Again I'm not sure what states would trigger that to test, though.

Ah, maybe if you upload https://github.com/harvard-lil/perma-filecheck/blob/develop/test_assets/eicar-standard-antivirus-test-file-adobe-acrobat-attachment.pdf?

rebeccacremona · 2024-10-07T16:21:07Z

perma_web/static/css/style-responsive.scss

 }

-// This appears to give us a sticky footer without breaking anything.
-// But I'm not sure about it.


LOL I love this comment.

rebeccacremona · 2024-10-07T16:35:10Z

perma_web/frontend/components/CreateLink.vue

 onMounted(() => {
+ globalStore.components.createLink = getCurrentInstance().exposed


I haven't seen this pattern before. Is it a way to let components communicate with each other?

Yeah I made this one up, but the thought process was ...

Vue components have this nice way to encapsulate inputs and outs -- defineProps shows you what settings the parent component can set and two-way data bindings, and defineExpose shows you what methods the parent can poke. (globalState pokes holes in that whole encapsulation, so we always use that by way of a necessary or convenient evil.)

So for the direct parent, communication is easy -- you embed <Child ref=child props...>, and use child.method() to poke child.

Oh but then it turns out some extended relative needs to poke child sometimes too. So parent will store it as globalState.childRef and the relative can do globalState.childRef.method(). Alternatively we can have relative update a globalState variable and child watch it, which gets encapsulated in "event bus" libraries -- but if we're only going to have one instance of Child, that feels like unnecessarily poking holes in Child's defineProps/defineExpose interface.

Then after repeating this a few times, we have globalState.childRef and globalState.kidInstance and globalState.widget and it starts to feel messy, so what if we tidy them all up under a globalState.components dictionary and have the children register themselves instead of making their parents do it?

I think all of this makes sense and ends up flowing pretty naturally, except that it's not really proper for the children to register themselves, because the parents are the ones in a position to know how many instances there are. So it'd probably be better to do const childRef = ref(); globalState.components.child = childRef in the parent. I can switch to that if you think it's worth updating.

(This would also have the virtue that globalState.components.child = childRef is easier to read than globalStore.components.me = getCurrentInstance().exposed)

Interesting! Thanks for taking the time to walk me through it. It does seem like a tidy alternative to "we can have relative update a globalState variable and child watch it", if maybe a bit idiosyncratic? But no objections here! No opinion on switching things so the parent registers the child/children or not; I think I understand your argument that having the parent do it is more correct; I could believe this way is easier read, if it's not spread across files, maybe?

globalState.components.child = childRef does look MUCH less fancy than globalStore.components.me = getCurrentInstance().exposed 🙂

jcushman added 12 commits October 6, 2024 15:43

Restyle capture button

e4a81b8

New form components

753cd7d

Refactor Spinner component

6bcc204

- takes up space in the DOM, easier to style - horizontally centered - uses single config object for spin.js params - default aria attributes

Refactor: consistent handling of component refs in globalStore

34e7b79

fetch wrapper improvements

00faa42

- fetchDataOrError accepts data option that can be either formData or json data - fetchDataOrError returns response data, if any, on error, and provides access to response object

Add working upload component

1d4698c

Fix keyboard focus on link row hide/show

5e8ceca

Fix drag object positioning for jstree

9c68312

body { height: 100% } causes event.pageX and event.pageY not to work, because body scrolls rather than window scrolling

Link batch refactor to use fetchDataOrError

8111b89

folder loading improvements

df721e5

- show spinner instead of unstyled text while loading - show "No folder selected" instead of empty div when no folder selected

Refactor createlink

7dd7c7d

- captureGUID and captureErrorMessage as props instead of global state - CreateLink uses fetch wrapper

update js build files

2abbcda

jcushman requested a review from a team as a code owner October 6, 2024 20:14

jcushman requested review from teovin and removed request for a team October 6, 2024 20:14

teovin requested review from rebeccacremona and removed request for teovin October 7, 2024 14:40

rebeccacremona approved these changes Oct 7, 2024

View reviewed changes

jcushman merged commit 945e8d5 into harvard-lil:develop Oct 7, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dashboard fixes and refactors #3614

Dashboard fixes and refactors #3614

jcushman commented Oct 6, 2024

codecov bot commented Oct 6, 2024

rebeccacremona left a comment

rebeccacremona Oct 7, 2024

jcushman Oct 7, 2024

rebeccacremona Oct 7, 2024

rebeccacremona Oct 7, 2024

jcushman Oct 7, 2024

rebeccacremona Oct 7, 2024

rebeccacremona Oct 7, 2024

rebeccacremona Oct 7, 2024

jcushman Oct 7, 2024

jcushman Oct 7, 2024

rebeccacremona Oct 7, 2024 •

edited

Loading

rebeccacremona Oct 7, 2024

		onMounted(() => {
		globalStore.components.createLink = getCurrentInstance().exposed

Dashboard fixes and refactors #3614

Dashboard fixes and refactors #3614

Conversation

jcushman commented Oct 6, 2024

codecov bot commented Oct 6, 2024

Codecov Report

rebeccacremona left a 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

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

rebeccacremona Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebeccacremona Oct 7, 2024 •

edited

Loading