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

Restyle SearchAddress focus and refactor #844

Merged
merged 3 commits into from
Jun 9, 2022
Merged

Conversation

lukaw3d
Copy link
Member

@lukaw3d lukaw3d commented Jun 1, 2022

Before After
before after

@github-actions
Copy link

github-actions bot commented Jun 1, 2022

MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 8 0 0.01s
✅ GIT git_diff yes no 0.01s
✅ TSX eslint 4 0 0 4.83s
✅ TYPESCRIPT eslint 2 0 0 4.48s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Merging #844 (43af684) into master (6fbc580) will decrease coverage by 0.05%.
The diff coverage is 85.71%.

❗ Current head 43af684 differs from pull request most recent head 0cec641. Consider uploading reports for the commit 0cec641 to get more accurate results

@@            Coverage Diff             @@
##           master     #844      +/-   ##
==========================================
- Coverage   87.88%   87.82%   -0.06%     
==========================================
  Files         101      102       +1     
  Lines        1634     1643       +9     
  Branches      337      337              
==========================================
+ Hits         1436     1443       +7     
- Misses        198      200       +2     
Flag Coverage Δ
cypress 57.59% <71.42%> (+0.14%) ⬆️
jest 74.74% <85.71%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/app/components/LoadingIndicator/index.tsx 100.00% <ø> (ø)
...omponents/Toolbar/Features/SearchAddress/index.tsx 91.66% <66.66%> (-4.34%) ⬇️
...nents/Toolbar/Features/SearchAddress/SearchBox.tsx 90.90% <90.90%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fbc580...0cec641. Read the comment docs.

/>
{value && (
<Button
style={{ marginLeft: '-48px', zIndex: 1, marginTop: '-2px', marginBottom: '-2px' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Two ideas:

  • we can avoid hardcoding all these pixels here , so component will be more 'stable' when updating Grommet or theme in the future
  • input height is not aligned with network selection button height, it's more visible on mobile view

something like this may work:

const FooButton = styled(Button)`
  margin-left: ${({ theme }) => `-${theme.global.edgeSize.large}`}; // avoid hardcoded pixels
  position: relative;
`
      <FooButton
        style={{
          visibility: value ? 'visible' : 'hidden',  // avoid jumping
        }}
        icon={<FormClose />}
        onClick={() => onClear()}
      />

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah, that's better!

I didn't know grommet's theme is in styled-components. Though theme.global.edgeSize.large isn't related. 48px comes from "x" button's width: left-padding + "x" icon width + right-padding.

Is there any benefit of position: relative; vs z-index: 1;?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any benefit of position: relative; vs z-index: 1;?
I have no idea 😄

@@ -1,6 +1,7 @@
import { Box, Button, TextInput } from 'grommet'
import { FormClose, Search } from 'grommet-icons/icons'
import React from 'react'
import styled, { useTheme } from 'styled-components'
Copy link
Contributor

Choose a reason for hiding this comment

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

useTheme is not used

Copy link
Member Author

Choose a reason for hiding this comment

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

oops :D

I looked at some linting options to detect these, and now I've got 204 other problems

@lukaw3d lukaw3d merged commit a6b6d32 into master Jun 9, 2022
@lukaw3d lukaw3d deleted the lw/refactor-search branch June 9, 2022 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants