Skip to content
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

Clean up all lint warnings #1199

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
fcdb525
Clean up a few lint warnings
snowystinger Oct 28, 2020
17d4031
Merge branch 'main' into clean-up-lint-warnings
snowystinger Oct 28, 2020
629adfe
Merge branch 'main' into clean-up-lint-warnings
snowystinger Nov 11, 2020
fb1c9e4
Merge branch 'main' of https://github.com/adobe/react-spectrum into c…
LFDanLu Dec 4, 2020
cb01f26
fixing some lint warnings
LFDanLu Dec 4, 2020
7656b73
more lint fixes
LFDanLu Dec 5, 2020
0fa07a6
fixing new story lint
LFDanLu Dec 5, 2020
4829591
Updating useOverlayPositon to not use dep array
LFDanLu Dec 7, 2020
e224f60
fixing rest of linter warnings
LFDanLu Dec 7, 2020
31d731b
fixing table test
LFDanLu Dec 8, 2020
2430934
Merge branch 'main' of https://github.com/adobe/react-spectrum into c…
LFDanLu Dec 8, 2020
afa543f
removing a whiteline
LFDanLu Dec 8, 2020
4696ec4
Merge branch 'main' into clean-up-lint-warnings
LFDanLu Dec 8, 2020
93e2f89
reverting lint max-level change
LFDanLu Dec 8, 2020
d6f3d79
Merge branch 'clean-up-lint-warnings' of https://github.com/adobe/rea…
LFDanLu Dec 8, 2020
9ba8140
fix breadcrumb layouteffect dep array
LFDanLu Dec 8, 2020
1d981ed
Merge branch 'main' of github.com:adobe/react-spectrum into clean-up-…
LFDanLu Feb 22, 2021
5e501f8
Merge branch 'main' of github.com:adobe/react-spectrum into clean-up-…
LFDanLu Apr 6, 2021
c8440b1
Merge branch 'main' into clean-up-lint-warnings
snowystinger Apr 9, 2021
874011a
Merge commit 'd04e70b27e40d0a04312d8c5e40127eceeefcd22' into clean-up…
snowystinger Jun 18, 2021
2166816
Merge branch 'main' into clean-up-lint-warnings
snowystinger Sep 1, 2021
2d273d0
Merge branch 'main' into clean-up-lint-warnings
snowystinger Sep 21, 2021
b4fa626
cleaning up more lint warnings
LFDanLu Sep 21, 2021
0081d9f
Merge branch 'main' into clean-up-lint-warnings
LFDanLu Sep 21, 2021
0d595fa
fix lint warnings
snowystinger Sep 23, 2021
ac9f2b4
Merge branch 'main' into clean-up-lint-warnings
snowystinger Sep 23, 2021
f21b763
Merge branch 'main' into clean-up-lint-warnings
LFDanLu Sep 23, 2021
ed616c4
Merge branch 'main' into clean-up-lint-warnings
snowystinger Jan 4, 2022
57c3437
Merge branch 'main' into clean-up-lint-warnings
majornista Feb 1, 2022
98ef135
Merge branch 'main' into clean-up-lint-warnings
majornista Feb 1, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/@react-aria/focus/src/useFocusable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

import {FocusableDOMProps, FocusableProps} from '@react-types/shared';
import {mergeProps} from '@react-aria/utils';
import React, {HTMLAttributes, MutableRefObject, ReactNode, RefObject, useContext, useEffect} from 'react';
import React, {HTMLAttributes, MutableRefObject, ReactNode, RefObject, useContext, useEffect, useMemo} from 'react';
import {useFocus, useKeyboard} from '@react-aria/interactions';

interface FocusableOptions extends FocusableProps, FocusableDOMProps {
Expand All @@ -32,7 +32,8 @@ interface FocusableContextValue extends FocusableProviderProps {
let FocusableContext = React.createContext<FocusableContextValue>(null);

function useFocusableContext(ref: RefObject<HTMLElement>): FocusableContextValue {
let context = useContext(FocusableContext) || {};
let context = useContext(FocusableContext);
context = useMemo(() => context || {}, [context]);

useEffect(() => {
if (context && context.ref) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ export function useInteractOutside(props: InteractOutsideProps) {
document.removeEventListener('touchstart', onPointerDown, true);
document.removeEventListener('touchend', onTouchEnd, true);
};
}, [onInteractOutside, ref, state.ignoreEmulatedMouseEvents, state.isPointerDown, isDisabled]);
}, [onInteractOutside, ref, state, isDisabled]);
}

function isValidEvent(event, ref) {
Expand Down
2 changes: 1 addition & 1 deletion packages/@react-aria/numberfield/src/useNumberField.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ export function useNumberField(props: NumberFieldProps, state: NumberFieldState,
handleInputScrollWheel
);
};
}, [inputId, isReadOnly, isDisabled, decrement, increment]);
}, [inputId, isReadOnly, isDisabled, decrement, increment, ref]);

let domProps = filterDOMProps(props, {labelable: true});
let {labelProps, inputProps} = useTextField(
Expand Down
3 changes: 3 additions & 0 deletions packages/@react-aria/overlays/src/useOverlayPosition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,12 @@ export function useOverlayPosition(props: AriaPositionProps): PositionAria {
crossOffset
})
);

// eslint-disable-next-line react-hooks/exhaustive-deps
}, deps);

// Update position when anything changes
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Member Author

Choose a reason for hiding this comment

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

Out of curiosity, is it just not checking in deps? or is deps actually missing some?
Lets add a comment when we // disable so that we know why we're ignoring it. It looks like you've started doing that a bit, so that's good. We should make sure every disable of this rule has an explanation

Copy link
Member

Choose a reason for hiding this comment

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

For this one, the linter states " React Hook useCallback was passed a dependency list that is not an array literal" and I tried facebook/react#14476 (comment) w/ the JSON.stringify but it broke a lot of stuff so I opted for disabling it. I'll go ahead and put an explanation though, good idea

Copy link
Member

Choose a reason for hiding this comment

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

actually, looking at it again maybe I can do away with the dep array since the useEffect below can have a dep on updatePosition

useEffect(updatePosition, deps);

// Update position on window resize
Expand Down
2 changes: 1 addition & 1 deletion packages/@react-aria/selection/src/useSelectableItem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export function useSelectableItem(options: SelectableItemOptions): SelectableIte
focusSafely(ref.current);
}
}
}, [ref, isFocused, manager.focusedKey, manager.isFocused, shouldUseVirtualFocus]);
}, [ref, isFocused, manager.focusedKey, manager.isFocused, shouldUseVirtualFocus, focus]);

// Set tabIndex to 0 if the element is focused, or -1 otherwise so that only the last focused
// item is tabbable. If using virtual focus, don't set a tabIndex at all so that VoiceOver
Expand Down
2 changes: 1 addition & 1 deletion packages/@react-aria/ssr/src/SSRProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export function useSSRSafeId(defaultId?: string): string {
console.warn('When server rendering, you must wrap your application in an <SSRProvider> to ensure consistent ids are generated between the client and server.');
}

return useMemo(() => defaultId || `react-aria-${ctx.prefix}-${++ctx.current}`, [defaultId]);
return useMemo(() => defaultId || `react-aria-${ctx.prefix}-${++ctx.current}`, [defaultId, ctx]);
}

/**
Expand Down
1 change: 1 addition & 0 deletions packages/@react-aria/virtualizer/src/ScrollView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ function ScrollView(props: ScrollViewProps, ref: RefObject<HTMLDivElement>) {
return () => {
clearTimeout(state.scrollTimeout);
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
ktabors marked this conversation as resolved.
Show resolved Hide resolved

let updateSize = useCallback(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export function useVisuallyHidden(props: VisuallyHiddenProps = {}): VisuallyHidd
} else {
return styles;
}
}, [isFocused]);
}, [isFocused, style]);

return {
visuallyHiddenProps: {
Expand Down
4 changes: 4 additions & 0 deletions packages/@react-spectrum/breadcrumbs/src/Breadcrumbs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,14 @@ function Breadcrumbs<T>(props: SpectrumBreadcrumbsProps<T>, ref: DOMRef) {
yield computeVisibleItems(newVisibleItems);
}
});
// Don't need to include childArray.length in dep array cuz children is already in array
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [listRef, children, setVisibleItems, showRoot, isMultiline]);

useResizeObserver({ref: domRef, onResize: updateOverflow});

// Don't need to include updateOverflow in dep array cuz children is already in array
// eslint-disable-next-line react-hooks/exhaustive-deps
useLayoutEffect(updateOverflow, [children]);

let contents = childArray;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

import {action} from '@storybook/addon-actions';
import {Breadcrumbs} from '../';
import {Button} from '@react-spectrum/button';
import {ButtonGroup} from '@react-spectrum/buttongroup';
// import {Heading} from '@react-spectrum/text';
import {Item} from '@react-stately/collections';
import React from 'react';
Expand Down Expand Up @@ -136,6 +138,10 @@ storiesOf('Breadcrumbs', module)
<Item>Root</Item>
</Breadcrumbs>
)
)
.add(
ktabors marked this conversation as resolved.
Show resolved Hide resolved
'adding breadcrumbs',
() => <DynamicBreadcrumbs />
);

function render(props = {}) {
Expand Down Expand Up @@ -179,3 +185,44 @@ function renderMany(props = {}) {
</Breadcrumbs>
);
}

let folders = [
{id: 1, label: 'Home'},
{id: 2, label: 'Trendy'},
{id: 3, label: 'March 2020 Assets'}
];

let DynamicBreadcrumbs = () => {
Copy link
Member

Choose a reason for hiding this comment

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

For testing the above Breadcrumb dep array changes

let [breadcrumbs, setBreadcrumbs] = React.useState(folders);
let addBreadcrumb = () => {
let newBreadcrumbs = [...breadcrumbs];
newBreadcrumbs.push({
id: breadcrumbs.length + 1,
label: `Breadcrumb ${breadcrumbs.length + 1}`
});

setBreadcrumbs(newBreadcrumbs);
};

let removeBreadcrumb = () => {
let newBreadcrumbs = [...breadcrumbs];
newBreadcrumbs.pop();
setBreadcrumbs(newBreadcrumbs);
};

return (
<div>
<Breadcrumbs>
{breadcrumbs.map(f => <Item key={f.id}>{f.label}</Item>)}
</Breadcrumbs>
<ButtonGroup marginEnd="30px">
<Button variant="secondary" onPress={() => addBreadcrumb()}>
Add Breadcrumb
</Button>
<Button variant="secondary" onPress={() => removeBreadcrumb()}>
Remove Breadcrumb
</Button>
</ButtonGroup>
</div>
);
};
16 changes: 9 additions & 7 deletions packages/@react-spectrum/utils/src/Slots.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*/

import {mergeProps} from '@react-aria/utils';
import React, {useContext, useMemo, useRef} from 'react';
import React, {useContext, useMemo} from 'react';

interface SlotProps {
slot?: string
Expand All @@ -33,18 +33,20 @@ export function cssModuleToSlots(cssModule) {
}

export function SlotProvider(props) {
let stableEmptyObject = useRef({});
let parentSlots = useContext(SlotContext) || stableEmptyObject.current;
let parentSlots = useContext(SlotContext);
let {slots = {}, children} = props;

// Merge props for each slot from parent context and props
let value = useMemo(() =>
Object.keys(parentSlots)
let value = useMemo(() => {
let slotObj = parentSlots || {};
return (
Object.keys(slotObj)
.concat(Object.keys(slots))
.reduce((o, p) => ({
...o,
[p]: mergeProps(parentSlots[p] || {}, slots[p] || {})}), {})
, [parentSlots, slots]);
[p]: mergeProps(slotObj[p] || {}, slots[p] || {})}), {})
);
}, [parentSlots, slots]);

return (
<SlotContext.Provider value={value}>
Expand Down