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

Update asset structure with webpacker gem #1962

Merged

Conversation

Bodacious
Copy link
Contributor

Updates the asset management changes in #1864 to include the latest changes from the development branch.

Copy link
Contributor

@briri briri 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 @Bodacious. We should go through all of the selectors to see if the rules are still applicable. We should also review the image assets and see if we still need to hang onto them in the codebase (e.g. I think that swf file is no longer needed).

Those can and should be handled by other tickets, so lets get this one merged after @xsrust reviews

and for production, please type:

```
npm run bundle -- -p
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to add the --no-watch arg as well, or mention it somewhere here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -0,0 +1,16 @@
module.exports = (number, index) => [
['just now', 'right now'],
['less than a minute ago', 'in %s seconds'],
Copy link
Contributor

Choose a reason for hiding this comment

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

These values are not in our localization files. We should probably look at how best to accomplish that. We've been using the getConstant method but maybe there's a better way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Want to create an issue for that?

@@ -112,7 +98,9 @@ class Application < Rails::Application
config.active_record.raise_in_transactional_callbacks = true

# Load Branded terminology (e.g. organization name, application name, etc.)
config.branding = config_for(:branding).deep_symbolize_keys
if File.exists?(Rails.root.join('config', 'branding.yml'))
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@import "font-awesome-sprockets";
@import "font-awesome";
@import "variables/*";
@import "utils/*";
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking this might be a copy/paste issue. You've imported utils twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Thanks.

@@ -0,0 +1,7 @@
@media (min-width: 768px) {
#admin-dropdown ul li a, #super-admin-dropdown ul li a {
line-height: 26px !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would probably be safe to convert this one over to .dropdown or one of the other generic bootstrap selectors

@briri
Copy link
Contributor

briri commented Oct 24, 2018

Probably worth merging this soon as its getting hard to review with all the different file changes.

@Bodacious
Copy link
Contributor Author

@briri @xsrust hold off for now, there are still a couple of loose ends I need to tie up here. Will make sure it merges properly

@Bodacious
Copy link
Contributor Author

@xsrust @briri This should be good to go!

@briri
Copy link
Contributor

briri commented Oct 31, 2018

bundle audit fails on a Loofah vulnerability that was just patched yesterday. flavorjones/loofah#154. I'll merge this and submit the update of that dependency (and an updated version number) in a separate PR

@briri briri merged commit 7a51724 into DMPRoadmap:development Oct 31, 2018
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.

2 participants