Skip to content
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

Client-side JSON import/export #119

Merged
merged 27 commits into from
May 3, 2020
Merged

Client-side JSON import/export #119

merged 27 commits into from
May 3, 2020

Conversation

psvenk
Copy link
Member

@psvenk psvenk commented Apr 21, 2020

Implements #74 at long last. Please see that issue and the commit messages for more details.

@psvenk
Copy link
Member Author

psvenk commented Apr 21, 2020

Some things to do before merging (from my conversation with @kdk1616):

  • On the login page, say "Use Aspen credentials or leave blank to import data"
  • Add a mouseover to each disabled term in the export dialog stating the reason why the term is disabled (term is not available on Aspen, or term is not included in the imported data)
  • Grey out inaccessible quarters in the quarter dropdown

@psvenk
Copy link
Member Author

psvenk commented Apr 21, 2020 via email

@psvenk
Copy link
Member Author

psvenk commented Apr 21, 2020

Also, further testing is needed regarding how switching currentTableData affects calculated grades (see termsReset) and different formats of GPA.

@Ruborcalor
Copy link
Member

Great stuff! Some ideas.

  1. When logging in and then importing data, the term selector doesn't fully reset. In the screenshot below I imported only two terms, but all four still appear available in the term selector. What do you think about indicating which ones were included in the import?
Term Selector
  1. The default behavior seems to be that when navigating to home.html, you are prompted for an import file, however I imagine that most users would prefer to be redirected back to the login.html page if their session timed out.

@psvenk
Copy link
Member Author

psvenk commented Apr 21, 2020 via email

@psvenk
Copy link
Member Author

psvenk commented Apr 22, 2020

  • On the login page, say "Use Aspen credentials or leave blank to import data"
  • Add a mouseover to each disabled term in the export dialog stating the reason why the term is disabled (term is not available on Aspen, or term is not included in the imported data)

Fixed in 7ec1869.

  • Grey out inaccessible quarters in the quarter dropdown

Fixed in 89a9c67.

  • If more than one object is in tableData (e.g. scraped data and imported data), then the quarter dropdown does not adjust on switching currentTableData.

Fixed in 4e87e1e.

Remaining things to do:

  • The default behavior seems to be that when navigating to home.html, you are prompted for an import file, however I imagine that most users would prefer to be redirected back to the login.html page if their session timed out.

  • Instead of using the HTML title element for tooltips, use CSS and/or JavaScript to make a more accessible alternative that works on mobile devices and works with assistive technologies. See MDN and Inclusive Components for more info.
    • This is a good opportunity to mention that the accessibility of Aspine is not optimal and that we should add semantic attributes (among other improvements) at some point.
  • Fix build-lite.sh to work properly after 05435d6.

@psvenk
Copy link
Member Author

psvenk commented Apr 22, 2020

Approval has been secured from @notrodes

@Aspine Aspine locked and limited conversation to collaborators Apr 22, 2020
@Aspine Aspine unlocked this conversation Apr 22, 2020
Copy link
Collaborator

@notrodes notrodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good work dudeman.

@psvenk
Copy link
Member Author

psvenk commented Apr 23, 2020

Another thing to fix: The version detection for imports currently only checks the major version. We need to warn the user if the minor version of the import is newer than the minor version of Aspine, letting them know that they should use a newer version of Aspine to view the file (presumably Aspine lite).

@psvenk
Copy link
Member Author

psvenk commented Apr 24, 2020

I would like to note that the solution employed in 89a9c67 is hacky in that it uses setTimeout to defer loading the term dropdown, and we should aim for a more sustainable solution in the future.

@psvenk
Copy link
Member Author

psvenk commented Apr 24, 2020

Note to self re hackiness of solution: this can be fixed by rewriting isAccessible to take an optional argument which specifies which terms are included in an import (in the case of imported data) and reads that argument if it is present.

We can probably get away with not passing in anything like that for scraped data as long as currentTableData.overview is populated. IIRC that is the only field that is checked for scraped data.

@psvenk
Copy link
Member Author

psvenk commented Apr 24, 2020

I would like to note that the solution employed in 89a9c67 is hacky in that it uses setTimeout to defer loading the term dropdown, and we should aim for a more sustainable solution in the future.

Fixed in d939809.

@psvenk
Copy link
Member Author

psvenk commented Apr 24, 2020

The default behavior seems to be that when navigating to home.html, you are prompted for an import file, however I imagine that most users would prefer to be redirected back to the login.html page if their session timed out.

Fixed in 3393b11.

@psvenk
Copy link
Member Author

psvenk commented Apr 24, 2020

Fix build-lite.sh to work properly after 05435d6.

Fixed in d61f403.

Remaining issues:

Instead of using the HTML title element for tooltips, use CSS and/or JavaScript to make a more accessible alternative that works on mobile devices and works with assistive technologies. See MDN and Inclusive Components for more info.

The version detection for imports currently only checks the major version. We need to warn the user if the minor version of the import is newer than the minor version of Aspine, letting them know that they should use a newer version of Aspine to view the file (presumably Aspine lite).

@psvenk psvenk force-pushed the json branch 5 times, most recently from d79358b to f6c3bf1 Compare April 30, 2020 15:52
@psvenk
Copy link
Member Author

psvenk commented May 2, 2020

The version detection for imports currently only checks the major version. We need to warn the user if the minor version of the import is newer than the minor version of Aspine, letting them know that they should use a newer version of Aspine to view the file (presumably Aspine lite).

Fixed in db0604d.

@psvenk
Copy link
Member Author

psvenk commented May 2, 2020

Instead of using the HTML title element for tooltips, use CSS and/or JavaScript to make a more accessible alternative that works on mobile devices and works with assistive technologies. See MDN and Inclusive Components for more info.

Fixed in 847ba33.

All of the blockers that have been identified so far have been fixed.

@Ruborcalor @kdk1616 @notrodes Please take a look. Is this ready to merge?

psvenk added 24 commits May 2, 2020 18:42
Also, make the AJAX call use the Promise interface instead of a callback;
this will be useful later when the option to enter Aspine without
signing in is added.
- Serve a default response from the backend if username or password was
  left blank
- Fix the clock and disable the Reports tab if username or password was
  left blank
This also includes a check that disables any terms in the term dropdown
that are not included in the imported data.
We can run this build script whenever we tag a new version and
distribute the resulting client-side-only version of Aspine. This is
useful for people who want to view a JSON file from Aspine 2.x after
compatibility breaks.

This commit also adapts some other code to work with the lite version.
Instead of hardcoding substitutions, make build-lite.sh accept a syntax
resembling that of the C preprocessor so that lite-specific changes can
be made more easily.

This commit also brings the lite-specific changes up to date with
recent changes to the frontend codebase.
Previously, it was just updating currentTableData.
Some of the classes relating to "custom-select" were used specifically
to refer to the GPA/term dropdown. They have now been renamed to include
"gpa" in their names and have been supplemented with generic equivalents.
This will make it easier to reuse CSS when another dropdown is added to
the top bar.
The dropdown is not yet functional.
These options were added in <#62>
as a server-side import/export feature, but they no longer work as
intended since lazy loading was added in
<#77>.
In any case, this is superseded by the client-side import/export feature
and the lite version.
Remove code duplication regarding modal dialogs.
- Instead of creating a dummy currentTableData object for non-scrapers
and calling it "Current Year", set tableData = [] and make the import
modal pop up automatically.

- Make sure that Aspine works as usual after importing data—this
includes things like reloading the schedule and clock when switching
currentTableData using the dropdown.
For the home page to redirect to the login page under some circumstances,
the user-facing endpoints had to be changed a bit.
"/home.html" is now "/" and "/login.html" is now "/login"
(redirects are in place).
Instead of simply disabling terms that have not yet been downloaded from
export, allow the user to select those terms and have their data
automatically downloaded and included in the export.
Clarify instructions on login page and give reasons for terms being
greyed out in export dialog
Selecting a different currentTableData now causes the quarter dropdown
to update.
The major and minor version numbers are both checked now, and users are
shown the place to download older/newer versions of Aspine (lite).
Instead of setting the HTML "title" attribute, use CSS to make tooltips
that work with mobile devices and assistive technologies.
Copy link
Member

@Ruborcalor Ruborcalor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to merge

@psvenk psvenk merged commit 01c6fde into master May 3, 2020
@psvenk psvenk deleted the json branch May 3, 2020 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants