Implement AlertDialog (#71) #87

do02reen24 · 2021-08-29T04:57:11Z

Description

[Implement] AlertDialog

Drafted and implemented AlertDialog.

Provides three types of dialog: alert, confirm, and prompt.

Please comment about the structure of Component.

Related Issues

Feature request for [AlertDialog] #71

Tests

I did not add the test yet.
This PR needs more implementation.

Checklist

Before you create this PR confirms that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
Run yarn test or yarn test -u if you need to update snapshot.
Run yarn lint
I am willing to follow-up on review comments in a timely manner.

hyochan

Overall this is greatly done!

Just a few small improvements I can suggest here!

hyochan · 2021-08-29T14:34:41Z

packages/AlertDialog/index.tsx

+  flex-grow: 1;
+  flex-shrink: 1;


Is there any reason not using flex: 1 instead?

Recently, there was an issue where UI appears differently by browser due to the change of policy that interprets flex : 1 as chrome update. I didn't use abbreviation to prevent similar issues from occurring in the future.

hyochan · 2021-08-29T14:37:49Z

packages/AlertDialog/index.tsx

+interface ColorProps {
+  color: string;
+}


The styling principle in dooboo-ui is not to expose each nested style prop and rather expose the style itself.
For example, ViewStyle, TextStyle...

hyochan · 2021-08-29T14:42:42Z

packages/AlertDialog/index.tsx

+    {
+      isOpen = false,
+      type = 'alert',
+      color = '#000000',


This is not a request, but I think we need to think about how we can support default theme in packages 🤔

hyochan · 2021-08-29T14:44:15Z

packages/AlertDialog/index.tsx

+`;
+
+type Styles = {
+  modalContainer?: StyleProp<ViewStyle>;


I'd recommend you to install types for this to support typescript for dev env.

"@types/react-native-modalbox": "^1.4.9"

Then I think you'd want to change this to

modal?: ModalProps['style'];

hyochan · 2021-08-29T14:46:33Z

packages/AlertDialog/index.tsx

+      <>
+        <Modal
+          isOpen={isOpen}
+          style={modalStyles.modal}


I think it is enough to use like below.

style={[{borderRadius: 4}, styles.modal]}

Note that default styles should come first!

hyochan · 2021-08-29T14:48:15Z

packages/AlertDialog/package.json

+  "types": "lib/index.d.ts",
+  "version": "0.0.1",
+  "license": "MIT",
+  "postinstall": "tsc",


Adding dooboolab-welcome postinstall would make dooboolab-welcome package installed meaningful 😅

hyochan · 2021-08-29T14:49:19Z

stories/packages/AlertDialogStories/index.tsx

+import {ContainerDeco} from '../../../storybook/decorators';
+import {storiesOf} from '@storybook/react-native';
+import styled from '@emotion/native';
+


I think we can make this bit better by giving some paddings or margins :)

hyochan · 2021-08-29T14:52:18Z

packages/AlertDialog/README.md

+
+```
+yarn add dooboo-ui
+```
+
+or
+


I think we need to remove this since this will be provided independently.

- Provides three types of dialog: alert, confirm, and prompt.

- improve style props - add @types/react-native-modalbox - add dooboolab-welcome postinstall - improve storybook example improvement - update readme

- reflect the figma design

hyochan

Great work overall! Hope you can take a look at few other reviews.

@yujong-lee You might want to check out how to support defaultTheme in separate package components 🤔 which is related to your recent work #105

hyochan · 2021-09-10T08:40:51Z

packages/AlertDialog/index.tsx

-        <ModalButton onPress={() => onPress(true)}>
-          <ButtonText color={color}>OK</ButtonText>
+        <ModalButton style={styles?.button} onPress={() => onPress(true)}>
+          <ButtonText style={styles?.buttonText}>OK</ButtonText>
        </ModalButton>
      </>
    );

    const renderPrompt = (


I'd like to point out here that renderAlert is not a function. If you want to provide this as a function I want to ask you to change it to function renderAlert = () => ....

Currently, it is a component then you should separate the component on the outer side. Then also don't forget to rename it to PascalCase.

hyochan · 2021-09-10T08:41:18Z

packages/AlertDialog/index.tsx

+          {type === 'alert' && renderAlert}
+          {type === 'confirm' && renderConfirm}
+          {type === 'prompt' && renderPrompt}


When changing render function correctly you might want to call it here renderAlert().

hyochan · 2021-09-10T08:44:57Z

packages/AlertDialog/index.tsx

 `;

 type Styles = {
-  modalContainer?: StyleProp<ViewStyle>;
+  modal?: ModalProps['style'];


Since modal is a parent component, I think this should go to style instead of styles.

Also in styles, I think buttonContainer, and container, modalInput can be included to customize styles.

hyochan · 2021-09-10T08:47:17Z

packages/AlertDialog/index.tsx

+          <ButtonText isAdditional style={styles?.buttonText}>
+            CANCEL
+          </ButtonText>


How do user customize this text? User might want to localize this!

hyochan · 2021-09-10T08:47:26Z

packages/AlertDialog/index.tsx

+          <ButtonText isAdditional style={styles?.buttonText}>
+            CANCEL
+          </ButtonText>


stories/packages/AlertDialogStories/index.tsx

yujonglee

I just left some suggestions. Hope it helps.

packages/AlertDialog/index.tsx

stories/packages/AlertDialogStories/index.tsx

- replace createRef -> useRef - renderAlert make FC - separate style, styles - destructure props, styles

- Update Readme - Implement renderPrimaryButton, renderAdditionalButton

codecov · 2021-09-16T14:53:16Z

Codecov Report

Merging #87 (e206938) into master (b69c16d) will increase coverage by 0.31%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   95.50%   95.81%   +0.31%     
==========================================
  Files          22       22              
  Lines         578      574       -4     
  Branches      265      261       -4     
==========================================
- Hits          552      550       -2     
+ Misses         26       24       -2

hyochan

You've worked really hard. Thank you for giving your precious time 🙏

hyochan added the 🎯 feature New feature label Aug 29, 2021

hyochan requested changes Aug 29, 2021

View reviewed changes

hyochan force-pushed the master branch from 22ffea6 to 02f93f6 Compare August 30, 2021 06:50

do02reen24 and others added 4 commits September 10, 2021 17:33

Implement AlertDialog (hyochan#71)

02d2cba

- Provides three types of dialog: alert, confirm, and prompt.

Improve AlertDialog

01c0fe7

- improve style props - add @types/react-native-modalbox - add dooboolab-welcome postinstall - improve storybook example improvement - update readme

Modify Alert Dialog Design

3fe8206

- reflect the figma design

Resolve yarn.lock

b7b8779

hyochan force-pushed the feat/AlertDialog branch from f145d66 to b7b8779 Compare September 10, 2021 08:37

hyochan requested changes Sep 10, 2021

View reviewed changes

yujonglee reviewed Sep 11, 2021

View reviewed changes

packages/AlertDialog/index.tsx Outdated Show resolved Hide resolved

packages/AlertDialog/index.tsx Show resolved Hide resolved

packages/AlertDialog/index.tsx Outdated Show resolved Hide resolved

stories/packages/AlertDialogStories/index.tsx Outdated Show resolved Hide resolved

do02reen24 added 3 commits September 13, 2021 17:22

Improve AlertDialog

ca0ffb1

- replace createRef -> useRef - renderAlert make FC - separate style, styles - destructure props, styles

Implement AlertDialog renderButtonFunction

2c00201

- Update Readme - Implement renderPrimaryButton, renderAdditionalButton

Fix AlertDialog CI build failed

e206938

yujonglee self-requested a review September 16, 2021 16:41

hyochan approved these changes Sep 18, 2021

View reviewed changes

hyochan merged commit 906c1c5 into hyochan:master Sep 18, 2021

hyochan mentioned this pull request Sep 18, 2021

Feature request for [AlertDialog] #71

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement AlertDialog (#71) #87

Implement AlertDialog (#71) #87

do02reen24 commented Aug 29, 2021

hyochan left a comment

hyochan Aug 29, 2021

do02reen24 Sep 4, 2021

hyochan Aug 29, 2021

hyochan Aug 29, 2021

hyochan Aug 29, 2021

hyochan Aug 29, 2021

hyochan Aug 29, 2021

hyochan Aug 29, 2021 •

edited

Loading

hyochan Aug 29, 2021

hyochan left a comment

hyochan Sep 10, 2021 •

edited

Loading

hyochan Sep 10, 2021

hyochan Sep 10, 2021

hyochan Sep 10, 2021

hyochan Sep 10, 2021

yujonglee left a comment

codecov bot commented Sep 16, 2021

hyochan left a comment

Implement AlertDialog (#71) #87

Implement AlertDialog (#71) #87

Conversation

do02reen24 commented Aug 29, 2021

Description

Related Issues

Tests

Checklist

hyochan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyochan Aug 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyochan left a comment

Choose a reason for hiding this comment

hyochan Sep 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujonglee left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 16, 2021

Codecov Report

hyochan left a comment

Choose a reason for hiding this comment

hyochan Aug 29, 2021 •

edited

Loading

hyochan Sep 10, 2021 •

edited

Loading