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.

Sign up for GitHub

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

Jump to bottom

Some improvements #1

Merged
merged 6 commits into from
Jun 8, 2021
Merged

Some improvements #1

merged 6 commits into from
Jun 8, 2021

Conversation

Windvis
Copy link
Contributor

@Windvis Windvis commented May 20, 2021

Good job! I only did some minor thingies.

Things I changed:

  • fix tiny syntax error which prevented the engine from booting
  • change the styling setup so appuniversum isn't included twice if the host app also imports it
  • include ember-cli-moment-shim. This would otherwise thrown an error if the host app didn't have that installed.
  • Setup the dummy app for easier engine development

@Windvis
Switch to the modern version of creating an engine
7149ae6 
This also fixes a build error due to invalid syntax.

https://github.com/ember-engines/ember-engines/pull/758/files?diff=split&w=1
@Windvis
Styling setup changes
f5b6561 
This prevents ember-cli-sass from compiling and adding the styles to vendor.css. This is needed to prevent the appuniversum styles from being added twice.

Apps can import the engine styles directly.
@Windvis Windvis requested a review from aatauil May 20, 2021 12:34
@Windvis
Install ember-cli-moment-shim as a dependency
d3fbc3b 
This should normally have been installed by ember-moment's blueprint.
@Windvis
Also display the engine inside the dummy app
e4de87c 
This makes it easier to test thing without having to run a different app.
aatauil
aatauil reviewed May 21, 2021
View reviewed changes
Copy link
Member

@aatauil aatauil left a comment

Choose a reason for hiding this comment

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

I tested it turning on lazy loading but the app crashed. I dont know if we really need it but I think its a nice extra for later

@aatauil aatauil self-requested a review May 21, 2021 10:33
aatauil
aatauil approved these changes May 21, 2021
View reviewed changes
Copy link
Member

@aatauil aatauil left a comment

Choose a reason for hiding this comment

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

All good and working and can be merged :)

@Windvis
Copy link
Contributor Author

Windvis commented May 21, 2021

I tested it turning on lazy loading but the app crashed. I dont know if we really need it but I think its a nice extra for later

Interesting. What did the error say?

@aatauil
Copy link
Member

aatauil commented May 21, 2021

I tested it turning on lazy loading but the app crashed. I dont know if we really need it but I think its a nice extra for later

Interesting. What did the error say?

Had something to do with "monkey-patch-ember-computed" cant really find what that is or does online for some reason

@Windvis
Copy link
Contributor Author

Windvis commented May 21, 2021

It seems to be a similar issue as this: sir-dunxalot/ember-tooltips#420

ember-macro-helpers also uses app.import to import that file, so that probably needs to be changed in a similar way to support lazy engines.

@aatauil aatauil merged commit 6eba700 into master Jun 8, 2021
@Windvis Windvis deleted the chore/improvements branch June 8, 2021 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aatauil aatauil aatauil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Windvis @aatauil