feat(popover): add align functionality #2835
Conversation
Pull Request Test Coverage Report for Build 4195
💛 - Coveralls |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e3825ad:
|
| bottom: ['tc', 'bc'], | ||
| 'bottom-left': ['tl', 'bl'], | ||
| 'bottom-right': ['tr', 'br'], | ||
| left: ['cr', 'cl'], |
There was a problem hiding this comment.
How come we mix and match the nomenclature of these attributes? Some have quotes and others don't. I understand that the ones that don't have them don't need them, but why not favor consistency?
Yeah, but your scientists were so preoccupied with whether or not they could, they didn't stop to think if they should.
There was a problem hiding this comment.
I guess the idea is "use it when it's needed don't when it's not".
There was a problem hiding this comment.
Yeah I think we would tend to follow juliens comment. Also we have an eslint rule in this project(quote-props) that auto formats it to not use quotes when it isn't necessary.
Here is an example eslint error
There was a problem hiding this comment.
The only reason we haven't changed it to "consistent-as-needed" yet:
liferay/eslint-config-liferay#39
is due to an open bug in Prettier:
Once that is resolved this will become somewhat nicer:
const ALIGNMENTS_MAP = {
'bottom': ['tc', 'bc'],
'bottom-left': ['tl', 'bl'],
'bottom-right': ['tr', 'br'],
'left': ['cr', 'cl'],
// etc...
};| ref = popoverRef as React.Ref<HTMLDivElement>; | ||
| } | ||
|
|
||
| React.useEffect(() => { |
There was a problem hiding this comment.
Is the preferred practice to use React.useSomething instead of importing useSomething from React, as I see examples of both when searching through the source code (mostly of React.useSomething)?
There was a problem hiding this comment.
TBH, it's not a huge issue if you go one way or the other, so long as its consistent for the component or file you are working in. Personally I would opt for React.* and I forget where this conversation had happened, but I think we decided to try an opt for React.* to avoid confusion with our other custom hooks use*.
There was a problem hiding this comment.
(Not this repo, but...) In liferay-portal we mostly just use useState and friends directly, because the number of built-in hooks is small and nobody actually gets confused about them coming from React. Less clutter in the code too, less likely to have to awkwardly wrap lines etc.
packages/clay-popover/src/index.tsx
Outdated
| /** | ||
| * React element that the popover will align to when clicked. | ||
| */ | ||
| alignNode?: React.ReactElement & { |
There was a problem hiding this comment.
It might be better to rename the prop to trigger, at first glance I thought I needed to just pass the element ref and will also create a consistency with DropDown that uses the same approach to alignment.
|
@matuzalemsteles updated |
fixes #2768