[DUOS-1759][risk=no] Updated step selector for data access request page #1787

lu-c · 2022-09-02T19:35:52Z

Lots of ugliness in the code to accomodate keeping the existing page as it is!

Have you read Terra's Contributing Guide lately? If not, do that first.

Label PR with a Jira ticket number and include a link to the ticket
Label PR with a security risk modifier [no, low, medium, high]
PR describes scope of changes
Get a minimum of one thumbs worth of review, preferably two if enough team members are available
Get PO sign-off for all non-trivial UI or workflow changes
Verify all tests go green
Test this change deployed correctly and works on dev environment after deployment

… request page - Added radio group option to forms - Added global styles (initial start) - Modified forms to extract some of the specific styling to the data submission form (that this page doesnt share)

Co-authored-by: Gregory Rushton <rushtong@users.noreply.github.com>

…er component >.<

…1752

…to change the normal select so it does not allow you to add a value, but later. Moved new researcher code to the existing researcher component.

…nt to change the normal select so it does not allow you to add a value, but later

… Removed explicit creatable formfieldType - it will now create if a creatableConfig is provided, otherwise, it will not allow the user to add new fields

…DUOS-1752 # Conflicts: # src/components/forms/forms.css # src/components/forms/forms.js

…DUOS-1752

… orig

…1752

…not saving value, converted input checkboxes to FormField components - will update the radio later :(

… the form components

connorlbark

the tabs themselves look good!

connorlbark · 2022-09-06T14:47:43Z

src/pages/DataAccessRequestApplication.js

+              }
+            }, [
+              h(Tab, {
+                // key: `dar-request-step-1`,


is this comment needed? if not, maybe delete.

…1752

…S-1759

lu-c · 2022-09-07T14:50:12Z

sorry, didnt read the ticket thoroughly enough, will continue work on the behavior of this today (this pr just focused on the UI)

1. position-stickyed the step bar 2. added window eventListener to watch for changes in scroll position and select the appropriate step. 3. One hiccup was that when you clicked on a section, it would slowly scroll there in the tab selector since the onScroll would update for the current position, overtaking the default where the tab selector would immediately change, and that ~.5s delay felt realy bad... so I added a debounce manually. Tried to do it through lodash, but it wasn't working, and I wasnt sure why.

lu-c · 2022-09-07T17:41:49Z

IT'S READY AGAIN, lots of changes this time - read the ticket more closely, this is lookin' pretty sweet ;)

lu-c · 2022-09-07T18:04:11Z

src/pages/DataAccessRequestApplication_new.js

@@ -215,6 +230,8 @@ class DataAccessRequestApplicationNew extends Component {
      prev.formData = merge(prev.formData, formData);
      return prev;
    });
+
+    window.addEventListener('scroll', this.onScroll);


Shoot, i didnt realize codacy wouldnt like this... I don't know how to do this part of the ticket without adding an event listener though. :[ Let me brainstorm

If codacy is giving us false positives here, we can change that. I'm not sure what the best practice is, but codacy shouldn't be the primary roadblock.

TIL you can bypass codacy warnings by adding disabling eslint on the same lines - would this be okay? XD I guess I'll let the reviewers weigh in hehe

There's nothing wrong with your function. It's a textbook definition of a debounce with a reset when the function is called repeatedly.

Codacy's issue is that it's not context aware of its warnings. Generally setTimeout is frowned upon for anything regarding data since defining a fixed time delay on async operations (where time to completion is indeterminate) can be dangerous. In this context though the UI changes are independent of anything else going on in the component, so it's not a problem.

If you care about Codacy warnings, try looking into lodash's (vanilla, not fp) debounce function and see if that does anything (though odds are it'll be doing the same things under the hood).

That said, I have no idea about eventListener warnings. If Codacy is giving you warnings about onScroll then you can ignore them.

…kirt codacy rules

…kboxes and radio components on the cloud use section

…tes behind env

…S-1759

connorlbark

This is really cool! It's very satisfying! There's some weird behavior around step 4 on my laptop (seems to prefer staying on 3 or 5) but clicking each of them works as expected and this is likely just some weirdness with heights and such.

JVThomas · 2022-09-12T12:28:45Z

src/pages/DataAccessRequestApplication_new.js

@@ -1062,7 +1094,12 @@ class DataAccessRequestApplicationNew extends Component {
              })
            ]),

-            div({ isRendered: this.state.step === 4 }, [
+            div({id: 'step-4', className: 'step-container'}, [


You don't have to worry about the Progress Report component right now. The mocks have it set up, but we won't be implementing the feature on the back-end for a while due to Data Submitter and the DAR Form UI redesign being a higher priority.

The funky behavior in the scroll is probably because the div is so small that the typical user's scroll movement will shift this component in and out of your designated windowTop threshold during the debounce period.

ok i will remove this for now

JVThomas

Overall the code looks good. I've left two comments (the Codacy related one is optional) for you to look over, but the left navbar is working smoothly.

lu-c · 2022-09-12T13:34:52Z

This is really cool! It's very satisfying! There's some weird behavior around step 4 on my laptop (seems to prefer staying on 3 or 5) but clicking each of them works as expected and this is likely just some weirdness with heights and such.

I think this is because step 4 is literally like 30 pixels large or something, so it's really hard to catch - when this component is fleshed out and implemented, we won't see this issue anymore! :)

…ids, which ... isnt ideal. So updated the methods to use class instead of id, since the ids should not have to be futzed with and tied to the step they show up in!

…1752

…S-1759

lu-c and others added 25 commits August 23, 2022 23:26

[DUOS-1752] Added initial attempt at step1 of the new dar application…

f4ef98b

… request page - Added radio group option to forms - Added global styles (initial start) - Modified forms to extract some of the specific styling to the data submission form (that this page doesnt share)

[DUOS-1752] fixed key issue

836e4ee

Update src/components/forms/forms.js

018bc6d

Co-authored-by: Gregory Rushton <rushtong@users.noreply.github.com>

[DUOS-1752] moved the new researcher components into the old research…

ad1670c

…er component >.<

Merge branch 'develop' of github.com:DataBiosphere/duos-ui into DUOS-…

ebfbf0d

…1752

[DUOS-1669] reorganized the css, added creatable select option. Want …

4560ebd

…to change the normal select so it does not allow you to add a value, but later. Moved new researcher code to the existing researcher component.

[common-forms] reorganized the css, added creatable select option. Wa…

dd284a5

…nt to change the normal select so it does not allow you to add a value, but later

[common-forms] added some of Shae's changes

eeaa610

[common-forms] reorganized files again

9188edf

[common-forms] fixed eslint

b890a18

[common-forms] removed unnecessary css liine

3e8fc04

[common-forms] split formComponents css into a separate file as well.…

2698dd4

… Removed explicit creatable formfieldType - it will now create if a creatableConfig is provided, otherwise, it will not allow the user to add new fields

codacy

fbde3e4

fixed unit tests and codacy

c00387b

Merge branch 'common-forms' of github.com:DataBiosphere/duos-ui into …

092d28f

…DUOS-1752 # Conflicts: # src/components/forms/forms.css # src/components/forms/forms.js

removed searchPlaceholder in favor of keeping "placeholder"

de7a2db

Merge branch 'common-forms' of github.com:DataBiosphere/duos-ui into …

4e13b79

…DUOS-1752

[DUOS-1752] Hooked up form components with actions and variables.

d5879c6

[DUOS-1752] moved code to a new component, reverted researcherInfo to…

50f6eda

… orig

[DUOS-1752] Added warning

11f4142

Merge branch 'develop' of github.com:DataBiosphere/duos-ui into DUOS-…

0c2c82a

…1752

Merge branch 'develop' of github.com:DataBiosphere/duos-ui into DUOS-…

68f1b78

…1752

[DUOS-1752] Added textarea to formComponents, fixed signing official …

7e749a0

…not saving value, converted input checkboxes to FormField components - will update the radio later :(

[DUOS-1752] added z index so the error message will overlay on top of…

8513a6c

… the form components

[DUOS-1759] added side bar selector

a468afb

lu-c requested a review from a team as a code owner September 2, 2022 19:35

[DUOS-1759] im sorry it's a mess

3ccdd64

connorlbark approved these changes Sep 6, 2022

View reviewed changes

lu-c changed the base branch from develop to DUOS-1752 September 6, 2022 18:38

Merge branch 'develop' of github.com:DataBiosphere/duos-ui into DUOS-…

794f7ac

…1752

lu-c added 3 commits September 6, 2022 18:22

Merge branch 'DUOS-1752' of github.com:DataBiosphere/duos-ui into DUO…

611260a

…S-1759

[DUOS-1759] updated so position:sticky will work!

c7c21d1

[DUOS-1759] converted tabs to an array, life is just easier this way lol

9f41f79

lu-c closed this Sep 7, 2022

lu-c reopened this Sep 7, 2022

lu-c commented Sep 7, 2022

View reviewed changes

lu-c added 5 commits September 7, 2022 14:18

[DUOS-1759] testing.... to see if eslint-disable-line allows you to s…

adc37e5

…kirt codacy rules

[DUOS-1759] testing.... to see if eslint-disable-line allows you to s…

becc20c

…kirt codacy rules

[DUOS-1752] disabled name, fixed default value not being set for chec…

4a1d625

…kboxes and radio components on the cloud use section

[DUOS-1752] updated form field id, updated routes to lock new dev rou…

1fc62ab

…tes behind env

Merge branch 'DUOS-1752' of github.com:DataBiosphere/duos-ui into DUO…

3ab139e

…S-1759

connorlbark approved these changes Sep 9, 2022

View reviewed changes

JVThomas reviewed Sep 12, 2022

View reviewed changes

JVThomas approved these changes Sep 12, 2022

View reviewed changes

lu-c added 7 commits September 12, 2022 09:52

[DUOS-1752] removed progress section, which meant updating the class …

1c936ed

…ids, which ... isnt ideal. So updated the methods to use class instead of id, since the ids should not have to be futzed with and tied to the step they show up in!

Merge branch 'develop' of github.com:DataBiosphere/duos-ui into DUOS-…

e62bb6c

…1752

[DUOS-1752] updated default value to undefined by default

91ecf7c

[DUOS-1752] moved related code into same div

4d908ce

codacy

eb86dcf

flipped direction of children

c48cb6a

Merge branch 'DUOS-1752' of github.com:DataBiosphere/duos-ui into DUO…

fb2cd48

…S-1759

Base automatically changed from DUOS-1752 to develop September 14, 2022 00:49

lu-c added 2 commits September 13, 2022 21:03

[DUOS-1752] fixed merge conflicts

4dd1216

[DUOS-1752] codacy

c9878d4

lu-c merged commit 875e43a into develop Sep 14, 2022

lu-c deleted the DUOS-1759 branch September 14, 2022 01:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DUOS-1759][risk=no] Updated step selector for data access request page #1787

[DUOS-1759][risk=no] Updated step selector for data access request page #1787

lu-c commented Sep 2, 2022

connorlbark left a comment

connorlbark Sep 6, 2022

lu-c commented Sep 7, 2022

lu-c commented Sep 7, 2022

lu-c Sep 7, 2022

rushtong Sep 7, 2022

lu-c Sep 7, 2022

JVThomas Sep 12, 2022 •

edited

Loading

connorlbark left a comment

JVThomas Sep 12, 2022 •

edited

Loading

lu-c Sep 12, 2022

JVThomas left a comment

lu-c commented Sep 12, 2022

[DUOS-1759][risk=no] Updated step selector for data access request page #1787

[DUOS-1759][risk=no] Updated step selector for data access request page #1787

Conversation

lu-c commented Sep 2, 2022

connorlbark left a comment

Choose a reason for hiding this comment

connorlbark Sep 6, 2022

Choose a reason for hiding this comment

lu-c commented Sep 7, 2022

lu-c commented Sep 7, 2022

lu-c Sep 7, 2022

Choose a reason for hiding this comment

rushtong Sep 7, 2022

Choose a reason for hiding this comment

lu-c Sep 7, 2022

Choose a reason for hiding this comment

JVThomas Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

connorlbark left a comment

Choose a reason for hiding this comment

JVThomas Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

lu-c Sep 12, 2022

Choose a reason for hiding this comment

JVThomas left a comment

Choose a reason for hiding this comment

lu-c commented Sep 12, 2022

JVThomas Sep 12, 2022 •

edited

Loading

JVThomas Sep 12, 2022 •

edited

Loading