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

Fix: Convert template to JSX (fixes #89) #90

Merged
merged 16 commits into from
Jan 16, 2023
Merged

Fix: Convert template to JSX (fixes #89) #90

merged 16 commits into from
Jan 16, 2023

Conversation

swashbuck
Copy link
Contributor

Fixes #89

Fix

  • Adds .jsx template and removes .hbs template
  • Removes resources_force_download helper. The download HTML property is now widely supported: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#browser_compatibility Do we still need to support IE or older mobile OS versions?
  • Moved helper functions to the .jsx template and removed the helper JS file. As far as I know, these functions were not used by any other plugins. So, they shouldn't need to be registered as Handlebars helpers that can be accessed outside this plugin.

@chris-steele
Copy link
Contributor

chris-steele commented Nov 29, 2022

In practice IE11 support has been dropped in most cases, but the framework and core plugin GH pages still state support for IE11, so I would assume that as a core plugin this one should continue to provide support. N.B. https://www.adaptlearning.org/index.php/documentation/ links to a spec which is very much out of date!

Copy link
Contributor

@chris-steele chris-steele left a comment

Choose a reason for hiding this comment

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

Great job @swashbuck, my only comment is that for a full conversion onFilterClicked should be incorporated into your React component.

@swashbuck
Copy link
Contributor Author

Thanks, @chris-steele . I've moved the event to the template and added the resourcesForceDownload() helper since this needs to support IE11.

templates/resources.jsx Outdated Show resolved Hide resolved
templates/resources.jsx Outdated Show resolved Hide resolved
templates/resources.jsx Outdated Show resolved Hide resolved
@swashbuck
Copy link
Contributor Author

@chris-steele I seem to have a problem getting a11y.focusFirst to work. Should I be using a ref hook or am I not using a11y.focusFirst properly?

Aside from that, do you think it would be worthwhile to make a new React component type for each resources__filter-btn? That would reduce the amount of repeated code.

@chris-steele
Copy link
Contributor

chris-steele commented Dec 7, 2022

You could create a child component for the buttons 👍 you could also do one for the resource-item to reduce the complexity of this component (I would recommend it if the resource models were dynamic as it would reduce the number of React renders, but in this case resource models are static AFAIK)

@swashbuck
Copy link
Contributor Author

You could create a child component for the buttons 👍 you could also do one for the resource-item to reduce the complexity of this component (I would recommend it if the resource models were dynamic as it would reduce the number of React renders, but in this case resource models are static AFAIK)

Thanks, @chris-steele ! Focus is now working. I'll work on creating the child components next.

@chris-steele
Copy link
Contributor

chris-steele commented Dec 14, 2022

@oliverfoster @cahirodoherty-learningpool @eleanor-heath @joe-allen-89 I'm hearing internally that we aren't supporting IE11 anymore in Adapt, can anyone weigh-in on this?

@cahirodoherty-learningpool
Copy link
Contributor

@oliverfoster @cahirodoherty-learningpool @eleanor-heath @joe-allen-89 I'm hearing internally that we aren't supporting IE11 anymore in Adapt, can anyone weigh-in on this?

Hi Chris, yeah it's my understanding that we we no longer officially support IE11, but we can allow support of it as an override setting. I think anything IE specific can/should be dropped going forward.

@swashbuck
Copy link
Contributor Author

I've created child components for the filter buttons and resource items.

I've also introduced a resourceTypes array that contains the filter categories. This should make it easier to add new categories if necessary. One of our clients likes to have several custom categories and doesn't use the default ones (document, link, media). This also reduces code repetition as the filter button component only appears once in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

👀

Copy link
Contributor

Choose a reason for hiding this comment

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

👀

@oliverfoster oliverfoster merged commit e4e426e into master Jan 16, 2023
@oliverfoster oliverfoster deleted the issue/89 branch January 16, 2023 10:54
github-actions bot pushed a commit that referenced this pull request Jan 16, 2023
## [5.3.3](v5.3.2...v5.3.3) (2023-01-16)

### Fix

* Convert template to JSX (fixes #89) (#90) ([e4e426e](e4e426e)), closes [#89](#89) [#90](#90)
@github-actions
Copy link

🎉 This PR is included in version 5.3.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

github-actions bot pushed a commit to redwoodperforms/adapt-contrib-resources that referenced this pull request Jul 10, 2023
# [5.1.0](v5.0.0...v5.1.0) (2023-07-10)

### Fix

* _globals schema nesting (adaptlearning#102) ([6343b0d](6343b0d)), closes [adaptlearning#102](https://github.com/redwoodperforms/adapt-contrib-resources/issues/102)
* Add custom types to readme, change 'category' references to 'type' for consistency (fixes adaptlearning#108) ([bbf57a8](bbf57a8)), closes [adaptlearning#108](https://github.com/redwoodperforms/adapt-contrib-resources/issues/108)
* Added gitignore for release automation (adaptlearning#81) ([7c6694c](7c6694c)), closes [adaptlearning#81](https://github.com/redwoodperforms/adapt-contrib-resources/issues/81)
* Added release automation (adaptlearning#80) ([e9b6923](e9b6923)), closes [adaptlearning#80](https://github.com/redwoodperforms/adapt-contrib-resources/issues/80)
* Allow custom types (fixes adaptlearning#103) (adaptlearning#104) ([bfaef6e](bfaef6e)), closes [adaptlearning#103](https://github.com/redwoodperforms/adapt-contrib-resources/issues/103) [adaptlearning#104](https://github.com/redwoodperforms/adapt-contrib-resources/issues/104)
* Bump http-cache-semantics from 4.1.0 to 4.1.1 (adaptlearning#95) ([5ae1de4](5ae1de4)), closes [adaptlearning#95](https://github.com/redwoodperforms/adapt-contrib-resources/issues/95)
* Convert template to JSX (fixes adaptlearning#89) (adaptlearning#90) ([e4e426e](e4e426e)), closes [adaptlearning#89](https://github.com/redwoodperforms/adapt-contrib-resources/issues/89) [adaptlearning#90](https://github.com/redwoodperforms/adapt-contrib-resources/issues/90)
* Header template does not work (fixes adaptlearning#99) (adaptlearning#100) ([55fae85](55fae85)), closes [adaptlearning#99](https://github.com/redwoodperforms/adapt-contrib-resources/issues/99) [adaptlearning#100](https://github.com/redwoodperforms/adapt-contrib-resources/issues/100)
* Labels for resources items are unnecessarily verbose ([6b51bbc](6b51bbc))
* Remove required title from contentobject schema (adaptlearning#98) ([6e27c74](6e27c74)), closes [adaptlearning#98](https://github.com/redwoodperforms/adapt-contrib-resources/issues/98)
* Remove unnecessary 'this' from currentTarget, remove component__inner class (fixes adaptlearning#91) ([835befc](835befc)), closes [adaptlearning#91](https://github.com/redwoodperforms/adapt-contrib-resources/issues/91)
* Semantic release action package versions ([e7d02d4](e7d02d4))
* Typo in template ([d90bd43](d90bd43))
* Update schema $anchor to reference resources (fixes adaptlearning#87) ([930034f](930034f)), closes [adaptlearning#87](https://github.com/redwoodperforms/adapt-contrib-resources/issues/87)
* Version numbers removed from Readme files ([292dd99](292dd99))

### New

* Add click event trigger (solves: adaptlearning#106) (adaptlearning#107) ([0d27f68](0d27f68)), closes [adaptlearning#106](https://github.com/redwoodperforms/adapt-contrib-resources/issues/106) [adaptlearning#107](https://github.com/redwoodperforms/adapt-contrib-resources/issues/107)
* Added contentObject specific resources (fixes adaptlearning#84) (adaptlearning#85) ([4df42fd](4df42fd)), closes [adaptlearning#84](https://github.com/redwoodperforms/adapt-contrib-resources/issues/84) [adaptlearning#85](https://github.com/redwoodperforms/adapt-contrib-resources/issues/85)
* Issue and pr project addition automation (refs adaptlearning/adapt_framework#3315) (adaptlearning#79) ([3657367](3657367)), closes [adaptlearning#79](https://github.com/redwoodperforms/adapt-contrib-resources/issues/79)

### Update

* Add missing aria attribution for tabs (adaptlearning#78) ([f3e3e1b](f3e3e1b)), closes [adaptlearning#78](https://github.com/redwoodperforms/adapt-contrib-resources/issues/78)

### Upgrade

* Bump yaml and semantic-release (adaptlearning#105) ([28b3873](28b3873)), closes [adaptlearning#105](https://github.com/redwoodperforms/adapt-contrib-resources/issues/105)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert template to JSX
4 participants