Finish updating global focus styles #2050
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ng a new style that 'beats' the default 2px
🦋 Changeset detectedLatest commit: e23f4ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
colebemis
reviewed
May 4, 2022
| @@ -41,6 +42,7 @@ const StyledRadio = styled.input` | |||
| cursor: pointer; | |||
|
|
|||
| ${props => props.disabled && `cursor: not-allowed;`} | |||
| ${getGlobalFocusStyles(0)}; | |||
There was a problem hiding this comment.
Is it possible to make the focus ring a circle for radios?
There was a problem hiding this comment.
There is, but I'm matching what we have in Primer CSS.
| @@ -37,6 +38,8 @@ const Page = styled.a` | |||
| transition-duration: 0.1s; | |||
| } | |||
|
|
|||
| ${getGlobalFocusStyles(0)}; | |||
There was a problem hiding this comment.
The focus ring on pages has a subtle gray border that makes it look a little "fuzzy":
There was a problem hiding this comment.
This is another thing I was keeping from Primer CSS.
colebemis
approved these changes
May 4, 2022
Merged
PrinceSumberia
pushed a commit
to PrinceSumberia/react
that referenced
this pull request
Aug 1, 2022
* updates focus styles that do not match the Primer CSS implementation * updates ThemeSwitcher to render ActionMenu instead of DropdownMenu * explicitly sets outline-offset in global focus styles instead of adding a new style that 'beats' the default 2px * update snapshots * fixes cross-browser issues for global focus styles * fixes unintentional laggy focus style update on SubNav * updates focus styles for UnderlineNav * DRY button focus styles * updates snapshots * adds changeset * fixes type error * revert themePreval snapshot update
siddharthkp
added a commit
that referenced
this pull request
Aug 3, 2022
* Fix typos * Namespace UnderlineNav (#2053) * namespace underline classes? * snippity snaps * Create quick-toys-fly.md * Version Packages (#2034) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Select component a11y fixes (#2038) * fixes disabled option colors for all browsers (Firefox had the most issues) * fixes custom arrow color for disabled state and Windows high contrast mode, and hacks around Firefox quirks * updates SelectInput stories to use FormControl * makes cursor behavior consistent for inputs * removes redundant ARIA attributes from <select> * updates tests and snapshots * adds changeset * fixes media query for forced colors (high contrast mode) * hacks around Firefox Windows high-contrast mode quirk * addresses a11y feedback * fixes linting * Finish updating global focus styles (#2050) * updates focus styles that do not match the Primer CSS implementation * updates ThemeSwitcher to render ActionMenu instead of DropdownMenu * explicitly sets outline-offset in global focus styles instead of adding a new style that 'beats' the default 2px * update snapshots * fixes cross-browser issues for global focus styles * fixes unintentional laggy focus style update on SubNav * updates focus styles for UnderlineNav * DRY button focus styles * updates snapshots * adds changeset * fixes type error * revert themePreval snapshot update * Tooltip: anchoredPosition + IconButton (#2006) * Add breaking story * Add memex story * use behaviors deploy preview * add tooltip triangle * update snapshot * add label tooltips for story * update @primer/behaviors to latest * lint: remove unused import * Refactor Tooltip * Use Tooltip in IconButton * Add triangle styles for all directions * Add docs * Added delay * change ReactElement to ReactNode * keep ReactElement * Add tests! * compatible types :) * Fix docs * update snapshots * update behaviors to next minor * update snapshots * Fix IconButton duplicate label * missed a spot! * Fix Button story with tooltip * Apply suggestions from code review Co-authored-by: Cole Bemis <colebemis@github.com> * fix alignment with span Co-authored-by: Cole Bemis <colebemis@github.com> * Implement NavList component (part 1) (#2058) * Create NavList component * Implement NavList subcomponents * Reset font weight of trailing visual * Export NavList from drafts * Update NavList docs * Update NavList.mdx * Add children to props type * Add NavList tests * Create breezy-cooks-destroy.md * Pass props to underlying nav element * Show divider between groups * Update snapshot * ThemeProvider: Fix theme sync with system settings (#2048) * disable theme decorator for themeprovider story * remove incorrect effect * Create wicked-boats-allow.md Co-authored-by: Mike Perrotti <mperrotti@github.com> * Move theme decorator into helper util (#2044) * Move theme decorator into helper util * fix height * is addons import the problem? * does this work better for consumer-tests? * yep, deep import doesn't work either * add option to disable theme decorator on a story level Co-authored-by: Mike Perrotti <mperrotti@github.com> * Add missing changelog for #2006! (#2069) * Create improved-tooltip.md * Create iconbutton-default-tooltip.md * typo lol * Implement NavList.SubNav (#2064) * wip subnav * Add active prop to ActionList.Item * Handle nested current item * Add simple storybook story * Update snapshot * Stub tests * Use NavList in PageLayout story * Add test case todo * Fix sx typescript error * Add NavList.Item test * Add subnav tests * Add support for as="button" to ActionList.Item * Add nested subnav test * Update docs * Use spacing primitive * Reset marginY for safari * Document active prop * Create strong-nails-sip.md * Update snapshots * Update issue templates for this repo to include redirects to pattern … (#2063) * Update issue templates for this repo to include redirects to pattern proposal and request issues * Delete component proposal issue template * Update pattern discussion URL * Fix ButtonGroup component and add docs (#2054) * Make ButtonGroup display inline-flex * Create documentation page for ButtonGroup * Update Button docs * Create six-mirrors-call.md * Update tests * Update stories * Update six-mirrors-call.md * ActionMenu: adjust width based on padding (#2080) * adjust width based on padding * update snapshost * Form component design tweaks (#2057) * adds color-scheme style so inputs pick up UA color scheme styles * crops toggle switch knob's shadow inside the toggle switch boundaries * does not stretch FormControl input child to fill parent width unless 'block' prop is passed, and passes missing TextInputWrapper props in the Select component * adds changeset * rm duplicate prop * NavList: Add `sx` prop (#2077) * Add support for `sx` prop to NavList components * Default sx to empty object * Create happy-brooms-swim.md * Update snapshot * Fix broken inline component links (#2082) * updates Link href paths that link to React components * adds changeset * Fix ActionMenu with IconButton (#2084) * Fix IconButton for ActionMenu * Add test case * NavList.Item: Add `as` prop (#2076) * Add as prop to NavList.Item * Add test case for react router link with subnav * Use DOM APIs to determine if subnav contains current item * Update snapshots * Remove "not implemented" warnings * Create dry-feet-attack.md * Fix merge issues * Update snapshot * Fixes `onRemove` console error coming from token components (#2087) * prevents onRemove prop from being passed from token components though to the HTML element * adds changeset * Fix overflow issues in PageLayout.Content (#2075) * Fix overflow issues in PageLayout.Content * Update PageLayout story * Prettier * Create gentle-countries-rest.md * Fix merge errors * Update src/PageLayout/PageLayout.tsx * Update snapshot Co-authored-by: Dusty Greif <dustin.greif@gmail.com> * Uncomment guard (#2091) * Revert accessible Tooltip + IconButton (#2092) * Revert "Fix ActionMenu with IconButton (#2084)" This reverts commit 3204599. * Revert 2069 + 2006 + 2084 * Version Packages (#2056) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Bump doctocat version (#2096) * Fix Dialog example (#2083) * import Dialog v2 instead of old Dialog * Revert "import Dialog v1 instead of old Dialog" This reverts commit 8ac1080. * move the new Dialog to drafts * undeprecate the legacy dialog * Update docs/content/drafts/Dialog.mdx Co-authored-by: Cole Bemis <colebemis@github.com> * update navigation menu * Create funny-hats-sing.md Co-authored-by: Cole Bemis <colebemis@github.com> * Set aria-multiselectable in SelectPanel (#2095) * Reduce storybook build time in CI to < 2 mins (#2093) * debug-ci-gatsby-build-speed * add storybook as well * remove storybook-addon-html * test: use storybook-addon-html, but disable babel-plugin-open-source * test: remove Pagination component * make room for storybook * try storybook-addon-turbo-build * bring back Pagination * test: optimizationLevel: 2 * debug: give more info during build! * rollback debuging changes * move addon-html back to production only Co-authored-by: Cole Bemis <colebemis@github.com> * Adds draft SegmentedControl docs (#2081) * adds a draft for the SegmentedControl component docs * addresses first round of PR feedback * changes 'block' prop name to 'fullWidth * Update docs/content/SegmentedControl.mdx Co-authored-by: Cole Bemis <colebemis@github.com> * Update docs/content/SegmentedControl.mdx Co-authored-by: Cole Bemis <colebemis@github.com> * Update docs/content/SegmentedControl.mdx Co-authored-by: Cole Bemis <colebemis@github.com> * addresses remainder of PR feedback Co-authored-by: Cole Bemis <colebemis@github.com> * Tests: Add tests for lockfile version (#2106) * Add tests for lockfile version * add resolveJsonModule for typecheck * Allow minor version updates for prod dependencies (#2117) * Allow minor version updates for prod dependencies * Create proud-colts-attend.md * chore(deps): bump @primer/octicons-react from 16.1.1 to 17.3.0 (#2119) Bumps [@primer/octicons-react](https://github.com/primer/octicons) from 16.1.1 to 17.3.0. - [Release notes](https://github.com/primer/octicons/releases) - [Changelog](https://github.com/primer/octicons/blob/main/CHANGELOG.md) - [Commits](primer/octicons@v16.1.1...v17.3.0) --- updated-dependencies: - dependency-name: "@primer/octicons-react" dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Export NavList from the main bundle (#2112) * Export ActionListDividerProps * Export NavList from the main bundle * Add NavList link to the nav * Create stale-hounds-notice.md * Add componentId * Fix CI for Node 16.15.1 (#2123) * update package-lock for node 16.15.1 * use Node 14 for docs * Use Node 14 for deploy production * Version Packages (#2097) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * ADR: Parallel drafts track & plan for deprecated components (#1722) * propose migration plan * Update adr-005.md * Update adr-005.md * move SelectMenu to the first list * Update with parallel track talk * Update adr-005.md * rename back to drafts * Update contributor-docs/adrs/adr-005.md Co-authored-by: Cole Bemis <colebemis@github.com> * Apply suggestions from code review Co-authored-by: Cole Bemis <colebemis@github.com> Co-authored-by: Leslie Cohn-Wein <lesliecdubs@github.com> * Remove component list from ADR Co-authored-by: Mike Perrotti <mperrotti@github.com> Co-authored-by: Cole Bemis <colebemis@github.com> Co-authored-by: Leslie Cohn-Wein <lesliecdubs@github.com> * ADR 004: Strict props or Composite components (#1703) * Add ADR for children as API * add adrs to eslint ignore list * editing phase 1 * add NewButton examples * lol title * Replace accidental Button usage * change title * add renderChild to the example * Apply suggestions from code review Co-authored-by: Leslie Cohn-Wein <lesliecdubs@github.com> Co-authored-by: Cole Bemis <colebemis@github.com> * Apply suggestions from code review Co-authored-by: Cole Bemis <colebemis@github.com> * Add decision * Update contributor-docs/adrs/adr-004-children-as-api.md * clarify ActionMenu example is from legacy version * removed sidenote because it feels like a tangent Co-authored-by: Leslie Cohn-Wein <lesliecdubs@github.com> Co-authored-by: Cole Bemis <colebemis@github.com> * Fixed an issue with ButtonDanger Not defined. (#2128) Replaced ButtonDanger with Button Component with the props variant="danger" Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> * Deprecate SideNav in favor of NavList (#2120) * Deprecate SideNav * Create small-donkeys-provide.md * Enforce correct semantics for TabNav (#2125) * Swaps nav and div and adds semantically correct roles * Fix tests, add body div and move sx * Adds changeset * Fixing TabNavProps export Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> * Adding codeql scanning (#2134) https://github.com/github/primer/issues/937 * Navlist passthrough action list group props (#2133) * passthrough actionlist group props to navlist.group * changeset * pass through props * fix pass through block * Update docs/content/NavList.mdx * avoid passing through action list props, but spread additional props to allow aria and data attributes through Co-authored-by: Cole Bemis <colebemis@github.com> * Explicitly declare workflow permissions for previews (#2137) * explicitly declare workflow permissions for previews * Basic SegmentedControl functionality (#2108) * implements basic SegmentedControl functionality * updates file structure * adds SegmentedControl to drafts * adds changeset * fixes TypeScripts issues * revert package-lock.json changes * fixes SegmentedControl tests and updates snapshot * style bug fixes * Update src/SegmentedControl/fixtures.stories.tsx Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> * improve visual design for hover and active states * ARIA updates from Chelsea's feedback * updates tests and snapshots * Ignore *.test.tsx files in build types * Use named export for SegmentedControl This fixes live code examples in the docs * Update package-lock.json * updates lock file * fixes checkExports test for SegmentedControl * design tweak for icon-only segmented control button Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> Co-authored-by: Cole Bemis <colebemis@github.com> * Use import for `focus-visible` instead of require (#2149) * CONTRIBUTING.md: Updating typecheck instructions (#2151) The `typecheck` script was renamed in the past in #1960, correcting the contribution docs * Add AutocompleteContext to Autocomplete component exports (#2153) * Autocomplete: allow "falsely" input values (#2154) * fix: allow autocomplete input value to be falsely * chore: added changeset * fixes linting error Co-authored-by: Erik Hughes <erik.hughes@outlook.com> * fix: checkbox to useIsomorphicLayoutEffect (#2155) Co-authored-by: Erik Hughes <erik.hughes@outlook.com> * Change false to undefined (#2122) * Add focusZone to TabNav (#2139) * Add focusZone to TabNav * Add aria-selected to tabs * Custom strategy to ensure selected tab is focused on re-entry * Add tests for new TabNav focus management * ActionMenu: a11y fixes (batch 1) (#2099) * Don't add aria-expanded=false * remove "Select a" from aria-label * remove tabIndex from AnchoredOverlay * update story to use interactive element * update snapshots * Create anchored-overlay-clever-mails-roll.md * update outdated label in test * Add describedby to milestone story * ActionMenu: Replace typeahead with mnemonics (#2105) * Replace typeahead with mnemonics * add aria-keyshortcuts as part of useMnemonics * Create gold-falcons-shake.md * support user configured aria-keyshortcuts * delete unused useTypeaheadFocus * Selected tab doesn't need to be <a> tag (#2158) * Selected tab doesn't need to be a tag * Updates tests for new code and updates snapshot * AnchoredOverlay: Keep tabIndex for renderAnchor (#2159) * Revert "remove tabIndex from AnchoredOverlay" This reverts commit e7f2d03. * remove change from changelog as well * update matching snapshots * Version Packages (#2131) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * SplitPageLayout API (#2144) * Draft SplitPageLayout API docs * Update docs/content/SplitPageLayout.mdx Co-authored-by: Vinicius Depizzol <vdepizzol@gmail.com> * Update docs/content/SplitPageLayout.mdx * visible -> hidden prop Co-authored-by: Vinicius Depizzol <vdepizzol@gmail.com> * Convert commonjs code to esm during build (#2150) * Build theme-preval to esm * Use default export for theme-preval * Create sweet-elephants-end.md * Update theme-preval snapshot * skip transform for storybook (#2177) * Correct and improve Storybook and docs for form controls (#2143) * updates checkbox props tables, storybook stories, and storybook controls * updates Radio props table, Storybook stories, and Storybook controls * updates FormControl Storybook stories, and Storybook controls * updates TextInput and TextInputWithWrapper props tables, Storybook stories, and Storybook controls * updates Select stories * fixes more issues with Select stories and prop docs * updates Textarea prop tables, Storybook stories, and Storybook controls * updates CheckboxGroup and RadioGroup Storybook stories, and Storybook controls * updates Autocomplete Storybook stories, and Storybook controls * fixes 'size' control in TextInput stories * fixes a11y issues with Autocomplete examples * updates input stories to use the FormControl component and controls, and updates sidebar to reflect that relationship * updates tests * adds changeset * fix linting issue * updates after merging from main * excludes story-helpers.tsx from build * fixes regression in CheckboxGroup and RadioGroup fixture stories * Bump primer primitives to latest `7.8.4` (#2180) * update version of primer primitives * update preval stamp * changeset * update snapshot * Improves keyboard navigation for the SegmentedControl (#2145) * implements keyboard navigation strategy for the SegmentedControl suggested by the a11y team, and warns component users if the control does not have a label * adds changeset * crappy fix for bug where useFocusZone breaks after selecting a button * corrects typo in test name * consolidates button focus test, and adds comment to focusInStrategy test * remove getNextFocusable by tweaking focusInStrategy Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> * Version Packages (#2161) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Updated StateLabel to include `issueClosedNotPlanned` and fix `issueClosed` colour (#2171) * Updated StateLabel * Add changeset * Update test * Update the test suites * Fix broken focus outline on the Select component (#2173) * fixes broken focus outline on the Select component * fixes linting issue * adds changeset * SegmentedControl variant prop (#2164) * renders a tooltip for icon-only segmented control buttons * implements responsive variant prop * adds tests * minor story tweaks * refactor useMatchMedia * adds useMatchMedia tests, fixes useMatchMedia bugs, updates SegmentedControl tests * removes aria attributes from Storybook controls * rm irrelevant 'TODO' comments * adds changeset * adds helpful comments * fixes button font-size in Safari * updates snapshots * addresses PR feedback * Update docs/content/SegmentedControl.mdx Co-authored-by: Cole Bemis <colebemis@github.com> * Update docs/content/SegmentedControl.mdx Co-authored-by: Cole Bemis <colebemis@github.com> * Update .changeset/pretty-students-judge.md Co-authored-by: Josep Martins <jsp.mrtns@gmail.com> * bumps @primer/primitives to version with segmented control variables * corrects storybook knobs to match current API * rm 'wide' key from 'variant' prop in props table * fix bad merge in SegmentedControl * adds more context to a11y issues with the tooltip implementation * adds changeset Co-authored-by: Cole Bemis <colebemis@github.com> Co-authored-by: Josep Martins <jsp.mrtns@gmail.com> * Use SegmentedControl component primitives (#2186) * bumps @primer/primitives and uses SegmentedControl component tokens * revert comment change in SegmentedControlIconButton * adds changeset * update themePreval snap with primitives@7.9.0 * PageLayout: Implement responsive `hidden` prop (#2174) * Implement responsive hidden prop * Write tests for useResponsiveValue * Document hidden prop * Update viewport range variables * Add hidden prop to header/footer/content * Update PageLayout tests * Create empty-garlics-clean.md * Fix lint errors * Button should respect polymorphic as prop (#2166) * update button to respect polymorphic as prop * update docs * update docs * fix doc * changeset * fix by removing sc as propr types * Add back link buttons stories using basic button Co-authored-by: Pavithra Kodmad <pksjce@github.com> * [passion week] Interaction tests with storybook's play function (#2172) * Add storybook play function for autocomplete * Add github workflow * Must run on pull-request and push to main * Package.lock revert * update workflow * configure static directories * Build storybooks correctly * A more complete autocomplete test * Fix up packge lock? * Fix up packge lock? * Add some documentation * Fix up autocomplete tests by adding data-testid * Codemod for react 18 children types (#2196) * run react 18 types codemod * run prettier * changeset * Upgrade `user-event` to v14 (#2190) * Upgrade `userEvent` to v14 * Fix Autocomplete tests Co-authored-by: Cole Bemis <colebemis@github.com> * PageLayout: Update `divider` prop responsive API (#2198) * Deprecate dividerWhenNarrow in favor of responsive values * Update PageLayout docs * Create spotty-parents-cheat.md * Update spotty-parents-cheat.md * Update snapshots * docs: NavList example with next/link's URL object (#2203) Adding another use-case in the NavList's NextJS example * PageLayout: Update `position` responsive prop API (#2199) * Update position prop API * Update snapshots * Update docs * Create fair-tips-travel.md * ActionMenu: Remove focus trap + Fix initial focus (#2024) * Revert "Revert "ActionMenu: Remove focus trap (#1984)" (#2023)" This reverts commit 866abc0. * prevent scroll when moving from anchor to menu * Add shortcut example from memex * progress! * make refs required * onclick should not interfere with keyboard * default to first item if anchor is not used * use userEvent instead of fireEvent for test to get full event * remedy merge * ignore containerRef for initial focus * update changeset to include initial focus on click * migrate to user-event@14 Co-authored-by: Cole Bemis <colebemis@github.com> * Default ConfirmationDialog to focusing `cancel` when the confirmation is a dangerous one (#2185) * allow configuration of the initial focus in a confirmation dialog * changeset * explicitly set the autoFocus when danger to cancel * intermediate value Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> Co-authored-by: Mike Perrotti <mperrotti@github.com> Co-authored-by: Katie Langerman <langermank@github.com> Co-authored-by: GitHub Design Systems Bot <30705008+primer-css@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> Co-authored-by: Cole Bemis <colebemis@github.com> Co-authored-by: Leslie Cohn-Wein <lesliecdubs@github.com> Co-authored-by: Dusty Greif <dustin.greif@gmail.com> Co-authored-by: y-tomida <fmtyd509@gmail.com> Co-authored-by: Hector Garcia <hectahertz@github.com> Co-authored-by: Dusty Greif <dgreif@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Karim K. Kanji <karimkkanji@cryosoft.co.ke> Co-authored-by: Owen Niblock <owenniblock@github.com> Co-authored-by: Jon Rohan <yes@jonrohan.codes> Co-authored-by: Matthew Costabile <mattcosta7@github.com> Co-authored-by: Rez <rezrah@github.com> Co-authored-by: Jeff Wilcox <427913+jeffwilcox@users.noreply.github.com> Co-authored-by: Will Glas <35304767+willglas@users.noreply.github.com> Co-authored-by: Erik Hughes <erik.hughes@outlook.com> Co-authored-by: Vinicius Depizzol <vdepizzol@gmail.com> Co-authored-by: Andrew L <anleac@github.com> Co-authored-by: Josep Martins <jsp.mrtns@gmail.com> Co-authored-by: Pavithra Kodmad <pksjce@github.com> Co-authored-by: Ian Sanders <iansan5653@github.com> Co-authored-by: Mardav Wala <wiinci@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates focus styles to match Primer CSS' styles created in this PR: primer/css#1744
Closes https://github.com/github/primer/issues/830
Screenshots
Please provide before/after screenshots for any visual changes
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.