-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix popover menu anchor alignment prop #19849
Changes from 11 commits
bbd951a
ad455dc
3008412
4a3db13
51f8691
0554e58
a397e99
44e316e
c5b6dec
0158ed7
a6e3d4c
b7f0c1c
b65dd2a
068c0b3
c0e29a3
22037c8
47b14b5
3476a4e
c2ce3bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import CONST from '../../CONST'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment related to the whole directory, not this specific line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think the one's directly in But i maybe wrong here since we also have bold and italic directly in /styles and not in /styles/utilities and vice versa. TBH idk why we have the styles/utilities in the first place it just seems unnecessary or if it is necessary then the styles folder's outside of it seems unnecessary.
Its a little ambiguous yes, maybe we could add a direction with it like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for not being clear. This style is related to the PopOver component, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, Thank you! I will be updating it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
export default (vertical) => ({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add JS Doc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
// On desktop app we are adding a header gap of 12px | ||
// which we need to add to vertical offset when setting | ||
// offset on desktop | ||
vertical: vertical + CONST.DESKTOP_HEADER_HEIGHT, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default (vertical) => ({vertical}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make changes in other places too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done