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

feat: design submission form #26

Merged
merged 23 commits into from May 16, 2019
Merged

feat: design submission form #26

merged 23 commits into from May 16, 2019

Conversation

Midnighter
Copy link
Contributor

@Midnighter Midnighter commented May 3, 2019

WIP:

  • Still need to add in new project addition option
  • Need to select a model based on organism
  • Make actual request to metabolic-ninja

Question:

  • I have a commented line because the AxiosResponse type annotation doesn't work. Does anyone know why?

@ChristianLieven ChristianLieven self-requested a review May 6, 2019 06:00
Copy link
Contributor

@ChristianLieven ChristianLieven left a comment

Choose a reason for hiding this comment

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

A few small things:

  • On the old platform both BiGG and Rhea are ticked when clicking on advanced
  • The choice of organism changes the available models in the advance view and if there is only one that one is selcted automatically

You might already be aware of these as this is WIP.

Other than that and my in-line comments this is looking good!

src/components/Design.vue Outdated Show resolved Hide resolved
src/components/Design.vue Outdated Show resolved Hide resolved
src/components/Models.vue Outdated Show resolved Hide resolved
@phantomas1234
Copy link
Contributor

@Midnighter can you please avoid ec7 becoming the default yeast model if you can because it doesn't work. We should integrate the yeast 8 model that has bigg IDs soon.

@Midnighter Midnighter changed the title WIP: design submission form feat: design submission form May 7, 2019
@Midnighter
Copy link
Contributor Author

I have now completed the design submission form. The mapping started in src/store/modules/models.ts needs to be extended. Also the model selection method could partially be moved to the models store, I guess.

I'd like to merge this for now since I need to work on the pitch.

Copy link
Contributor

@ChristianLieven ChristianLieven 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 to me except for that one error I'm having that it doesn't show the available models after selecting an Organism and a Project.

I couldn't test the submission process.

src/components/Design.vue Outdated Show resolved Hide resolved
Copy link
Member

@kvikshaug kvikshaug left a comment

Choose a reason for hiding this comment

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

I have some concerns about typing that I think should be addressed.
I've also made a handful of comments on both design and technical matters.

src/components/Design.vue Outdated Show resolved Hide resolved
src/components/Design.vue Outdated Show resolved Hide resolved
.env Outdated Show resolved Hide resolved
src/components/Design.vue Outdated Show resolved Hide resolved
src/components/Design.vue Outdated Show resolved Hide resolved
src/components/Design.vue Outdated Show resolved Hide resolved
src/components/Design.vue Outdated Show resolved Hide resolved
src/components/Design.vue Outdated Show resolved Hide resolved
src/components/Design.vue Outdated Show resolved Hide resolved
src/components/Design.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@ChristianLieven ChristianLieven left a comment

Choose a reason for hiding this comment

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

I had a few very minor comments left but overall agree and approve!

src/router.ts Outdated Show resolved Hide resolved
src/store/modules/models.ts Show resolved Hide resolved
src/views/Design.vue Show resolved Hide resolved
Copy link
Member

@kvikshaug kvikshaug 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 - I added two comments. I think the model object vs id should be fixed, while the rules issue is not important. Other than that, I still don't approve of us defining types from 3rd party libraries, but I guess I'm in the minority on this.

src/views/Design.vue Outdated Show resolved Hide resolved
src/views/Design.vue Outdated Show resolved Hide resolved
@Midnighter
Copy link
Contributor Author

@kvikshaug I've addressed your comments. I'm not 100% happy with the field validation yet but the hint is a decent solution for now. Not willing to spend any more time on it.

I've also removed next type annotation. I agree that we're not in full control of what that type actually is, so we shouldn't declare it ourselves. I've left in the types regarding rules, though.

@Midnighter Midnighter merged commit f5c61e8 into devel May 16, 2019
@Midnighter Midnighter deleted the feat-design branch May 16, 2019 10:35
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.

None yet

4 participants