Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Conversation

@liweitian
Copy link
Contributor

Description

Storage should not be involved in creating a new dialog. Currently, the new dialog model is shared with the new creation project flow which should be avoided.

Task Item

#1215

Type of change

Please delete options that are not relevant.

  • Code refactor (non-breaking change which improve code quality, clean up, add tests, etc)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have functionally tested my change

Screenshots

Please include screenshots or gifs if your PR include UX changes.

@liweitian liweitian changed the base branch from stable to master October 28, 2019 14:06

import { name, description } from './styles';

export function DialogModal(props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's good to see that, you are towards the direction to make more pure component.

But the interface here needs a bit rework, for example,

think about what's your "DialogModal“ component is designed for, then looks at this props.

Multiple problems, what do you mean by form? What should in the form? if your form only holds one data field, it's probably should be called field?

then this "updateForm, getErrorMessage", if you look at the naming, it's not in the same level, i think you mean onChange, onError?

But you know, if you delegate onChange, there should not be an on Error, because onChange tells everything, if you want to design an onError, you want to put a validator here, right?

Let's talk about this tomorrow.

@liweitian liweitian changed the title refactor creation flow refactor creation flow[WIP] Oct 28, 2019
@liweitian liweitian closed this Oct 30, 2019
@cwhitten cwhitten deleted the factorCreationFlow branch March 6, 2020 00:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants