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

Design app fixes after Webpacker installation #7935

Merged
merged 10 commits into from May 7, 2021
Merged

Conversation

ferblape
Copy link
Contributor

@ferblape ferblape commented May 5, 2021

🎩 What? Why?

This PR fixes a couple of issues related with #7733

  1. The app needed to be updated to use Webpacker new configuration, thus the files have been added
  2. The README and some docs need to be updated accordingly
  3. A test has been added to check some of the webpacker configuration (the package.json and it's lock file) are in synch with Decidim

As a summary, now the process is a bit long:

  1. bundle install
  2. npm install
  3. bin/rails s
  4. (optional) bin/webpack-dev-server

📌 Related Issues

Testing

You should be able to see the design app working again 😅

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

📷 Screenshots

Please add screenshots of the changes you're proposing
Description

♥️ Thank you!

@ferblape ferblape requested a review from mrcasals May 5, 2021 12:57
mrcasals
mrcasals previously approved these changes May 5, 2021
@mrcasals
Copy link
Contributor

mrcasals commented May 5, 2021

Actually, @ferblape, why not just commit the files? Wouldn't that be safer for anyone using this app? It feels strange that I need to manually run the command to install Webpacker to the application when the app is already there 😕

@ferblape
Copy link
Contributor Author

ferblape commented May 5, 2021

I see your point, it's similar to the Gemfile case, both the design app and decidim share the Gemfile and it's commited, right?

I didn't want to commit it to avoid the files to be outdated, but I guess that's something that can be prevented with a test or a check in the release process. What do you think?

@mrcasals
Copy link
Contributor

mrcasals commented May 6, 2021

Yeah, sounds good!

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 41187 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 41190 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 41193 lines exceeds the maximum allowed for the inline comments feature.

@ferblape
Copy link
Contributor Author

ferblape commented May 7, 2021

@mrcasals ready to review again. PR description updated with the new approach and updated docs.

Also, please check a test I've added in system/ to verify package.json is synchronized 😄

@mrcasals
Copy link
Contributor

mrcasals commented May 7, 2021

Nice, looks good to me!

@mrcasals mrcasals merged commit 0c50c53 into develop May 7, 2021
@mrcasals mrcasals deleted the design-app-details branch May 7, 2021 12:22
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.

None yet

2 participants