SIS Import Data Modal #193

rbiz4 · 2021-04-10T17:30:20Z

No description provided.

However, the modal does not show up. This is a WIP

The initial version of the SIS import data modal is completed: it shows up by default upon entering the advising page, and soon we will add the feature to make it not show up automatically when there is data to render.

rbiz4 · 2021-04-10T21:17:52Z

For now, the modal shows up automatically, as soon as the advising page opens up, as below

My guess is that we want this to stop when there is actually data to render from SIS.

Currently, there are two ways to make the modal automatically show up upon opening the advising page:

Method one (what I am using right now): I call this.props.triggerSISImportDataModal() in top_bar_advising.jsx, as shown below:

Method two (I am not using this): I set the visibility to true in the modal reducer, as shown below:

This gives us two ways to control whether/not the modal shows up automatically upon opening the advisor page.

My question is, should the automatic pop-up control be something that the page itself is responsible for (Method one), or is it something that the Modal should be responsible for (Method two)?

miaboloix · 2021-04-13T16:47:19Z

using just the toggle function is fine for now i think

miaboloix · 2021-04-13T17:01:23Z

static/js/redux/ui/modals/SIS_import_data_modal.jsx

+                    (or something like this). Click the
+                    button below to be taken to the Consent
+                    Page on SIS.</h3>
+            </div>);


Instead, I would write something like: "In order to connect with your advisors through Semester.ly, you must agree to Semester.ly importing your data from SIS." And then have the "Import SIS" button.

Will revise

miaboloix · 2021-04-13T17:01:44Z

static/js/redux/ui/semesterly.jsx

@@ -159,7 +159,6 @@ class Semesterly extends React.Component {
        <IntegrationModalContainer />
        <TutModalContainer />
        <PeerModalContainer />
-        <SaveCalendarModalContainer />
        <FinalExamsModalContainer />


Is this intentional?

This was accidental, will add it back.

miaboloix · 2021-04-13T17:03:27Z

static/js/redux/ui/top_bar_advising.jsx

+                    <span>SIS Import Data Modal</span>
+                </ReactTooltip>
+            </div>
+        );


I think this is okay for now but add a TODO comment to remove this and instead add an "Import Data" button. Then, create an Issue to track this TODO. Maybe this issue can be called "Replace star with an Import Data button that triggers the Import Data modal"

Will change button

I noticed that Semesterly makes use of Fontawesome.com icons if you want to look into that, although I think just a button that says "import data" would be fine

Changed SIS import data modal text Added the save calendar modal back to semesterly.jsx Changed the button that is used to pull up the modal from a star to actual text

JJamesWWang · 2021-04-14T04:38:07Z

static/js/redux/ui/top_bar_advising.jsx

@@ -30,6 +32,7 @@ class TopBarAdvising extends React.Component {
        this.comments_collapsed = 'neutral';
        this.toggleComments = this.toggleComments.bind(this);
        this.renderUserForPrint = this.renderUserForPrint.bind(this);
+        this.props.triggerSISImportDataModal()


Wouldn't this make it always pop up?

Okay, I saw your comment that it's like this for now. Maybe add a # TODO then

Hopefully this will fix the merge conflict

modal. These TODOs will be tracked with issues

Import data button CSS fixes

miaboloix

Pull most recent cs310 branch and also check for lint errors (npm run lint).

yesterd3v · 2021-04-14T15:44:44Z

Merge in cs310
Update: Disregard merging in cs310, looks like you did merge in yesterday.

(but yep, fix lint errors!)

miaboloix · 2021-04-14T16:06:30Z

Lint errors!

…esterly into SIS-data-modal

…into SIS-data-modal

JJamesWWang · 2021-04-14T18:46:03Z

I get a weird scrollbar when it first appears.

JJamesWWang · 2021-04-14T18:46:56Z

The modal also doesn't appear when I go to /advising

JJamesWWang

See above comments

rbiz4 · 2021-04-14T18:56:50Z

I commented out the auto-pop-up part, and I left a TODO, linked to an issue (#219) about it

rbiz4 · 2021-04-14T19:00:09Z

The scrollbar happens with other (pre-existing) modals as well

miaboloix

lgtm

Created file SIS_import_data_modal.jsx

ed11c92

rbiz4 requested review from alihacks, yesterd3v, JJamesWWang, miaboloix and jwfung April 10, 2021 17:30

rbiz4 linked an issue Apr 10, 2021 that may be closed by this pull request

Create UI for Import SIS Data modal #189

Closed

rbiz4 added 3 commits April 10, 2021 16:02

I added the star button to call up the modal

627603a

However, the modal does not show up. This is a WIP

Completed initial SIS import data modal

5131eaa

The initial version of the SIS import data modal is completed: it shows up by default upon entering the advising page, and soon we will add the feature to make it not show up automatically when there is data to render.

Fixed merge conflict

9010456

JJamesWWang linked an issue Apr 10, 2021 that may be closed by this pull request

Add Advisor login flow (first time advisor login) #188

Closed

JJamesWWang removed a link to an issue Apr 12, 2021

Add Advisor login flow (first time advisor login) #188

Closed

miaboloix requested changes Apr 13, 2021

View reviewed changes

Made changes according to Mia's comments

6e92133

Changed SIS import data modal text Added the save calendar modal back to semesterly.jsx Changed the button that is used to pull up the modal from a star to actual text

JJamesWWang reviewed Apr 14, 2021

View reviewed changes

rbiz4 added 6 commits April 14, 2021 02:54

Made tabs 2 spaces instead of 4

076143e

Hopefully this will fix the merge conflict

more spacing changes

1e9c3a9

yet more spacing changes

05a91e4

yet more spacing changes, specifically on advising.jsx

3f8eae2

Merge branch 'cs310-spring21' into SIS-data-modal

5d4986d

Added TODOs for the import data button and the SIS import data

1e38e20

modal. These TODOs will be tracked with issues

This was referenced Apr 14, 2021

Make triggerSISImportDataModal pop up when there is no user data #219

Closed

Move import data button to optimal position #220

Closed

miaboloix and others added 2 commits April 14, 2021 11:17

css fixes

1037719

Merge pull request #2 from jhuopensource/import-modal-button-fixes

2e82fe8

Import data button CSS fixes

miaboloix approved these changes Apr 14, 2021

View reviewed changes

miaboloix requested changes Apr 14, 2021

View reviewed changes

rbiz4 added 3 commits April 14, 2021 13:30

Merge branch 'cs310-spring21' of https://github.com/jhuopensource/sem…

1b61792

…esterly into SIS-data-modal

Merge branch 'SIS-data-modal' of https://github.com/rbiz4/semesterly …

43c26c1

…into SIS-data-modal

Fixed Lint issues

7bcca80

JJamesWWang requested changes Apr 14, 2021

View reviewed changes

JJamesWWang approved these changes Apr 14, 2021

View reviewed changes

miaboloix approved these changes Apr 14, 2021

View reviewed changes

rbiz4 merged commit 0884d12 into jhuopensource:cs310-spring21 Apr 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SIS Import Data Modal #193

SIS Import Data Modal #193

rbiz4 commented Apr 10, 2021

rbiz4 commented Apr 10, 2021 •

edited

Loading

miaboloix commented Apr 13, 2021

miaboloix Apr 13, 2021

rbiz4 Apr 13, 2021

miaboloix Apr 13, 2021

rbiz4 Apr 13, 2021

miaboloix Apr 13, 2021

rbiz4 Apr 13, 2021

jwfung Apr 14, 2021

JJamesWWang Apr 14, 2021

JJamesWWang Apr 14, 2021

miaboloix left a comment

yesterd3v commented Apr 14, 2021 •

edited

Loading

miaboloix commented Apr 14, 2021

JJamesWWang commented Apr 14, 2021

JJamesWWang commented Apr 14, 2021

JJamesWWang left a comment

rbiz4 commented Apr 14, 2021

rbiz4 commented Apr 14, 2021 •

edited

Loading

miaboloix left a comment

SIS Import Data Modal #193

SIS Import Data Modal #193

Conversation

rbiz4 commented Apr 10, 2021

rbiz4 commented Apr 10, 2021 • edited Loading

miaboloix commented Apr 13, 2021

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

miaboloix left a comment

Choose a reason for hiding this comment

yesterd3v commented Apr 14, 2021 • edited Loading

miaboloix commented Apr 14, 2021

JJamesWWang commented Apr 14, 2021

JJamesWWang commented Apr 14, 2021

JJamesWWang left a comment

Choose a reason for hiding this comment

rbiz4 commented Apr 14, 2021

rbiz4 commented Apr 14, 2021 • edited Loading

miaboloix left a comment

Choose a reason for hiding this comment

rbiz4 commented Apr 10, 2021 •

edited

Loading

yesterd3v commented Apr 14, 2021 •

edited

Loading

rbiz4 commented Apr 14, 2021 •

edited

Loading