Skip to content

Vir 1644 update formulaic ember js#57

Open
Ronnie-Odima wants to merge 14 commits intomasterfrom
VIR-1644-update-formulaic-ember-js
Open

Vir 1644 update formulaic ember js#57
Ronnie-Odima wants to merge 14 commits intomasterfrom
VIR-1644-update-formulaic-ember-js

Conversation

@Ronnie-Odima
Copy link
Contributor

No description provided.

Comment on lines +10 to +45
- [Git](https://git-scm.com/)
- [Node.js](https://nodejs.org/) (with npm)
- [Ember CLI](https://cli.emberjs.com/release/)
- [Google Chrome](https://google.com/chrome/)

## Installation

* `nvm use`
* `npm install`
* `bower install`
- `git clone <repository-url>` this repository
- `cd ember-formulaic`
- `ember g ember-cli-sass`
- `npm install`

## Running / Development

* `nvm use`
* `ember serve`
- `npm run start`
- Visit your app at [http://localhost:4200](http://localhost:4200).
- Visit your tests at [http://localhost:4200/tests](http://localhost:4200/tests).

### Code Generators

Make use of the many generators for code, try `ember help generate` for more details

### Running Tests - TK
### Running Tests

* `ember test`
* `ember test --server`
- `npm run test`
- `npm run test:ember -- --server`

### Building - TK: I generally don't differentiate the build created by `ember serve` from `ember build`
### Linting

* `ember build` (development)
* `ember build --environment production` (production)
- `npm run lint`
- `npm run lint:fix`

### Building

- `npm exec ember build` (development)
- `npm run build` (production)
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on these instructions? It's not actually too clear what I actually need to do to build the ember project?

We're modifying the dockerfile to install node, but then don't do anything else? Is the intention to be able to build the project inside of docker or not?

Can I have a more step by step guide to building the project?

Copy link
Contributor

@BryanTheScott BryanTheScott left a comment

Choose a reason for hiding this comment

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

This looks good so far. I still have a lot of files to go through, but I see a lot of clear improvements in addition to general platform upgrades. I'm going to keep working through this over the next couple of days, but I'm at a stopping place for now. I am submitting the few comments I have so far.

Copy link
Contributor

Choose a reason for hiding this comment

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

No change in this file, just removing a newline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this change necessary?

* `npm install`
* `bower install`
(Install node v20 using nvm)
- `nvm install 20`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep the .nvmrc and use nvm use here?

@attr('string') value;
@attr('number') position;
@belongsTo('optionlist', { async: false, inverse: 'options' }) list;
@belongsTo('optiongroup', { async: false, inverse: 'options' }) group;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is modeled correctly. An option can be in more than one optionlist. Or is it modeled differently here (compared to Django) on purpose?

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.

3 participants