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

Refactor index.js to webcomponents #3498

Merged
merged 79 commits into from
May 2, 2022
Merged

Refactor index.js to webcomponents #3498

merged 79 commits into from
May 2, 2022

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Mar 24, 2022

This pull request contains a major refactor of what used to be index.js.

It also replaces uglifier by terser to minify js.
This was a requirement because uglifier failed to compile lit components that use ref, which this pr uses for the search field. Terser is also more futureproofed as it supports ES6 and is more actively developed.

Refactor of index.js

Why?

  1. it was outdated:
  • It was still written in js instead of typescript
  • It used no longer supported packages (Typeahead
  • it still relied heavily on jquery
  1. It has grown out of the scope it was originally intended for, with code being added on top without proper refactor
  2. State was being handled by html (Tags which were or were not present)
  3. Multiple new features will be added that interact with this feature (see Rework filtering and sorting #3458)

New structure

Everything is centered around the class SearchQuery defined in search.ts
This class represents the state of the query parameters. It contains two maps of query parameters (one with single values and one with an array of values), which follow the subscriber scheme, allowing others to listen to changes in these maps;

The class already adds a first listener itself, that preforms a new search every time a query parameter changes.

Different webcomponents are created that interact with this state:

FilterButton

This is a very basic component that replaces a clickable token (and sets a given query param to a given value)

The same file also contains FilterIcon which replaces the filter icons in tables (and sets the query param filter to a given value). This components uses the above component as it is a basic extension.

SearchActions

This represents the optional dropdown to the right of the searchbar.
It contains searchoptions which directly interact with SearchQuery.
It also has searchactions which often use SearchQuery to get the current url params.

SearchField

This component represents the search field. It directly manipulates the filter parameter, but it also has search suggestions which interact with their own set of query parameters.
The search suggestion are effectively a custom implementation of the old typeahead module.

Peek.2022-04-29.11-57.mp4

SearchTokens

These are the tokens displayed above the searchbar. These now act completely separate from the search field and only interact with the SearchQuery state.

Search dropdowns

These were develop in their own pr as webcomponents, and adjusted in this pr to use the searchQuery class. See #3494

Other impacted places

course.js contained some special functionality to filter using tabs. I did not refactor this to components, but dit updete the code to use the SearchQuery state.

The page numbering for remote fetch (using will paginate defined in app/helpers/application_helper.rb) was not working correctly with our query parameters (even in existing code). Concretely it did not update the 'page' query param. I made an update to correct this.

A lot of rails views have been updated to use the new components.

@jorg-vr jorg-vr added the feature New feature or request label Mar 24, 2022
@jorg-vr jorg-vr self-assigned this Mar 24, 2022
@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2022

This pull request introduces 3 alerts when merging a72a059 into 595f0d7 - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Mar 31, 2022

This pull request introduces 3 alerts when merging cb08873 into 595f0d7 - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Apr 1, 2022

This pull request introduces 4 alerts when merging 40557d2 into 595f0d7 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Apr 1, 2022

This pull request introduces 4 alerts when merging 887303f into 595f0d7 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Apr 1, 2022

This pull request introduces 4 alerts when merging cdc0620 into 595f0d7 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Apr 1, 2022

This pull request introduces 4 alerts when merging fb644da into 595f0d7 - view on LGTM.com

new alerts:

  • 4 for Unused variable, import, function or class

@jorg-vr jorg-vr marked this pull request as ready for review April 29, 2022 10:08
@bmesuere bmesuere temporarily deployed to naos May 1, 2022 16:28 Inactive
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

This looks very good 👍

The headers in the autocomplete are more clear now and the left alignment of the tokens, search and selects is good. To me, the chips are a bit too close to each other (especially compared to the generous spacing between the selects below). The spec mentions 8px in between chips but I couldn't find the current value.

  • fix spacing between chips

Given this refactor, how hard would it be to store the selected filters for the exercise search within a course (on the series edit page) in local storage and restore them if present? In this case, I think teachers mostly use the same settings to find exercises (certain programming language/repo/language) in a certain course so this would speed things up for them.


For the sorting, I'm not convinced of the current solution. Clicking the header instead of the icon is easier but by removing the icon it becomes less discoverable. In addition, the lack of showing the default sort seems suboptimal. Would it be possible to manually add the default sort order (in code) so we could show the correct icon in the respective column? This would also improve the discoverability: if an arrow is shown next to a column header, this hints that you can sort them.

When sorting for the first time, the addition of a sort icon increases the header height and makes the entire header jump. (Adding the default order would solve this)

Do we need the third "blank" toggle item? This seems to make ik more complicated.

Because the icon is in front of the title, the label jumps to the right when hovering over it. I can see 2 possible fixes: reserve some blank space in front that's only filled in on hover/sort (but then the left alignment of the titles with the content breaks) or add the icon to the right of the title (against the spec but this seems acceptable).

If I understood correctly, you added the sort here as a proof of concept? We might have to discuss where to add sorting and where not. For example, sorting all submissions (or even all submissions within a large course) can take a lot of time. Because of the size of this PR, is it an option to disable sorting for now, get this merged and solve this in a subsequent PR?

  • move sorting to different pr

@jorg-vr
Copy link
Contributor Author

jorg-vr commented May 2, 2022

It would be very doable to to store selected filters in local storage. An easy solution would be to store the last used search url, and use it for the existing setbasurl method to initialize search params on a new page load without search params.

I'll make an issue for this

@jorg-vr jorg-vr changed the title Refactor index.js and add sort buttons to tables Refactor index.js to webcomponents May 2, 2022
@jorg-vr jorg-vr requested a review from bmesuere May 2, 2022 08:44
@bmesuere
Copy link
Member

bmesuere commented May 2, 2022

I also think this removes the last major dependency on jQuery!

app/assets/javascripts/components/search_field.ts Outdated Show resolved Hide resolved
app/assets/javascripts/components/search_token.ts Outdated Show resolved Hide resolved
app/assets/stylesheets/components/dropdown.css.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/components/search_field.css.scss Outdated Show resolved Hide resolved
app/assets/stylesheets/components/search_field.css.scss Outdated Show resolved Hide resolved
Copy link
Member

@chvp chvp left a comment

Choose a reason for hiding this comment

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

On the version that is currently deployed to Naos, course member label edit does not look good.

@jorg-vr jorg-vr temporarily deployed to naos May 2, 2022 12:46 Inactive
@jorg-vr jorg-vr temporarily deployed to naos May 2, 2022 13:09 Inactive
@jorg-vr jorg-vr requested a review from chvp May 2, 2022 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants