Skip to content
This repository was archived by the owner on Mar 19, 2021. It is now read-only.

feat(ui): popups ui-components #45

Closed
wants to merge 6 commits into from

Conversation

avetalia
Copy link
Contributor

Popups added

@avetalia avetalia requested a review from sergeysova March 15, 2019 00:05
@sergeysova
Copy link
Member

На первый взгляд не надо выносить PopupButton отдельно, так как его переиспользуемость равна единице.
Да и вместо него можно было обычную кнопку заюзать.

@avetalia
Copy link
Contributor Author

Перенесла атом PopUp к кнопке.
В рендере у PopUpButton обычная кнопка, поясни пожалуйста что ты имел в виду

@avetalia
Copy link
Contributor Author

PopUpButton перенесла к карточке (пока со своим стейтом), теперь это просто кнопка. PopUp остался в организмах

>
PopUp
</Button>
{!!popup && (
Copy link
Member

Choose a reason for hiding this comment

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

You don't need !! here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a discussion on React Telegram about that. I saw this !! in few popup examples, people said that it is reinsurance for some cases. Please confirm that I need to delete it.

Copy link
Member

Choose a reason for hiding this comment

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

You can delete it

Do you absolutely sure you want to delete? Just kidding we are
archiving them anyway.
</p>
<Button onClick={() => true}>Yes</Button>
Copy link
Member

Choose a reason for hiding this comment

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

Just call props.onDeleteClick

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am waiting for API with a real delete function, and alerts are blocked by husky, so this is a temporary dummy. Could I keep it?

Copy link
Member

Choose a reason for hiding this comment

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

Use defaultProps


const PopUpButton = () => {
const [popup, setPopup] = useState(false)

Copy link
Member

Choose a reason for hiding this comment

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

I think we should create const here:

const close = () => setOpened(opened => !opened)

And use close fn in onClose prop in PopUp

setPopup(() => false)
}}
>
<Box popup>
Copy link
Member

Choose a reason for hiding this comment

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

I think that button and <Box popup> should be inside PopUp component

CLOSE (should be x icon in the corner)
</Button>

<H3>Do you want to delete?</H3>
Copy link
Member

Choose a reason for hiding this comment

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

Also, we can create a Confirm component. Usage example:

<Confirm
  onCancel={close}
  onConfirm={deleteCard}
  text="Do you absolutely sure you want to delete?"
/>

box-shadow: 0 1px 3px 0 rgba(0, 0, 0, 0.2);
//transform: scale(1.001);
}
${({ sticky }) =>
Copy link
Member

Choose a reason for hiding this comment

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

You really need sticky here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to replace Card-atom with Box-atom later. Keep name "card" just for feature-components and Box as styled component. So later I am going to remove sticky from Card with whole Card-atom.


export const PopUp = ({ children, onClose }) => {
const ref = useRef(null)
const onDocumentClick = (event) => {
Copy link
Member

Choose a reason for hiding this comment

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

Please use useCallback here.

because in useEffect on unmount used new function, but new function do not removed

@avetalia
Copy link
Contributor Author

All changes were done in #46.

@sergeysova sergeysova closed this Mar 28, 2019
@sergeysova sergeysova deleted the feat/add-new-ui-components-popups branch September 8, 2019 12:08
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.

2 participants