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

WIP: Enable and use import maps #3209

Merged
merged 27 commits into from
Feb 22, 2023
Merged

WIP: Enable and use import maps #3209

merged 27 commits into from
Feb 22, 2023

Conversation

heyapricot
Copy link
Collaborator

@heyapricot heyapricot commented Oct 25, 2022

Resolves #3199

Description

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Documentation update

How Has This Been Tested?

Screenshots

* Load all javascript needed to run the AdminLTE theme and
* all the interactions.
*/
import 'admin-lte'
Copy link
Collaborator

Choose a reason for hiding this comment

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

@heyapricot i was able to make the sidenav work again by importing it here after pinning it.

@edwinthinks
Copy link
Collaborator

@heyapricot I've made some updates since the last week. Mainly:

The next step I'd take is to make sure the system tests start passing again. Pick one or a few and try to get it to start passing by adding JS that may have been left out from https://github.com/rubyforgood/human-essentials/blob/main/app/assets/javascripts/application.js or need to be moved from the webpack version.

This is exciting!

@edwinthinks
Copy link
Collaborator

@dorner as requested here is a list of things that still need to be fixed.

  • Icons on the side nav are missing. Likely due to needing to migrate fontawesome from webpacker.
  • select2 stimulus controller is not loading properly without error. Seems to be caused by the $ syntax not being recognized.
  • [Limited knowledge here from me] The bar code scanner code probably does not work. I don’t know how to test it either. I believe Scott has a barcode scanner.
  • There is logic to open a model in the new donations page when selecting the manufacturer or driver is new. This functionality hasn’t been moved over.
  • Date range filters doesn’t appear to work. It looks like in the webpacker there are libraries to handle that

This list isn't all encompassing, these are just the most upfront issues I've seen so far.

Happy to work with you to divide on conquer this if it makes sense :)

@cielf
Copy link
Collaborator

cielf commented Jan 22, 2023

FWIW, I believe you can use your phone as a bar code scanner. I don't know the details on that, though.

@dorner
Copy link
Collaborator

dorner commented Jan 23, 2023

You can use your computer's camera as well. If you click / tap in the barcode area it should automatically prompt you to use it.

@@ -112,7 +110,7 @@ gem "clockwork"
gem "mini_racer", "~> 0.6.3"
gem "nokogiri", ">= 1.10.4"
gem "image_processing"
gem "sprockets", "~> 4.2.0"
gem "sprockets", "~> 4.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Had to downgrade this due to rails/sprockets#749 .

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the system didn't like this downgrade very much :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

In what way?

@dorner
Copy link
Collaborator

dorner commented Feb 13, 2023

oh build is failing. That's weird, it wasn't doing that before :o Let me see if just running bundle will fix

@dorner
Copy link
Collaborator

dorner commented Feb 13, 2023

Should be fixed now.

@dorner dorner removed the WIP label Feb 19, 2023
Daniel Orner added 2 commits February 19, 2023 11:08
This only happens for the purchases -> new vendor popup. No idea why.
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

I don't know about the rest of the change, but I'd like to see the hack heavily commented.

@dorner dorner merged commit ca29b29 into main Feb 22, 2023
@dorner dorner deleted the implement-import-maps branch February 22, 2023 13:46
Copy link
Collaborator

@cielf cielf 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. Maybe we should put a tech debt issue in to solve it, though?

@dorner
Copy link
Collaborator

dorner commented Feb 22, 2023

Honestly, it would be so far down in priority that I kind of hope nobody ever picks it up. We have a lot better fish to fry - for example, at some point we probably should upgrade to Bootstrap 5, which might render this moot depending on if there are fixes in there.

@awwaiid
Copy link
Collaborator

awwaiid commented Feb 23, 2023

Thank you for getting this merged!

@cielf
Copy link
Collaborator

cielf commented Mar 26, 2023

@heyapricot @dorner @edwinthinks Thanks for your work on this -- we just released it!

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.

Move from Webpack to importmaps or esbuild
5 participants