Skip to content

Conversation

@mykalmax
Copy link
Member

No description provided.

@mykalmax mykalmax requested a review from vchan February 15, 2023 19:00
return { ok: false, detail: 'Empty response' }
if (response.status === 204)
return { ok: true, ...(await response.json()) }
if (response.status > 400)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be >= 400?

headline,
tagline,
description,
children,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does children need to be in PropsModalConfirmation? Is it already in React.HTMLAttributes?

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to define children as prop

}

export interface WithConfirmation {
setConfirmation: (confirmation: Confirmation | undefined) => void
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the below equivalent?

Suggested change
setConfirmation: (confirmation: Confirmation | undefined) => void
setConfirmation: (confirmation?: Confirmation) => void

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

project?: DirectoryApi
}): JSX.Element {
const directory = useMemo(() => new ModelDirectory(project), [project])
const [confirmation, setConfirmation] = useState<Confirmation | undefined>()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Confirmation a type and/or an interface? Can you create an instance of it? Does this mean confirmation can also be undefined? Can it also be null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Confirmation is an interface it only has meaning to typescript and we cant create an instance from typescript interfaces.
so confirmation is just optional and should be either object -> Confirmation or undefined from typescript point of view it should not be a null but at runtime it can be null it wont break or show an error

}): JSX.Element {
const directory = useMemo(() => new ModelDirectory(project), [project])
const [confirmation, setConfirmation] = useState<Confirmation | undefined>()
const [showCofirmation, setShowConfirmation] = useState(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [showCofirmation, setShowConfirmation] = useState(false)
const [showConfirmation, setShowConfirmation] = useState(false)

.then(created => {
if (isFalse((created as any).ok)) {
console.warn([
`Diroctory: ${directory.path}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`Diroctory: ${directory.path}`,
`Directory: ${directory.path}`,

.then(response => {
if (isFalse((response as any).ok)) {
console.warn([
`Diroctory: ${directory.path}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`Diroctory: ${directory.path}`,
`Directory: ${directory.path}`,

setConfirmation({
headline: 'Removing Directory',
description: `Are you sure you want to remove the directory "${directory.name}"?`,
yesText: 'Yes, Remove',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this looks better?

Suggested change
yesText: 'Yes, Remove',
yesText: 'Yes, remove.',

@mykalmax mykalmax merged commit 5764457 into main Feb 15, 2023
@mykalmax mykalmax deleted the max/ui_modal_component branch February 15, 2023 22:43
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