chore(content-explorer): Migrate Header#3887
Conversation
greg-in-a-box
force-pushed
the
content-explorer-search
branch
3 times, most recently
from
e144539 to
f3b56a1
Compare
| const isSearch = view === VIEW_SEARCH; | ||
|
|
||
| return ( | ||
| <div className="be-header"> |
There was a problem hiding this comment.
make sure the styles such as background and font size are using blueprint's token to support future customizability.
| aria-label={searchMessage} | ||
| data-testid="be-Header-searchInput" | ||
| <SearchInput | ||
| className="my-class" |
| data-testid="be-Header-searchInput" | ||
| <SearchInput | ||
| className="my-class" | ||
| defaultValue="" |
There was a problem hiding this comment.
does it need a default value?
There was a problem hiding this comment.
checked defaultValue is not a required prop so im gonna remove it
| // eslint-disable-next-line react/prop-types | ||
| const Header = ({ isHeaderLogoVisible = true, view, isSmall, searchQuery, onSearch, logoUrl, intl }: Props) => { | ||
| const { formatMessage } = intl; | ||
| const search = ({ currentTarget }: { currentTarget: HTMLInputElement }) => onSearch(currentTarget.value); |
There was a problem hiding this comment.
can we double check that this behavior is the same? i.e. onSearch is now passed directly to the component rather than needing this wrapper
There was a problem hiding this comment.
verified that the value passed into onSearch function is exact value I typed into the searchbar.
| view: View; | ||
| } | ||
|
|
||
| // eslint-disable-next-line react/prop-types |
There was a problem hiding this comment.
why does it need this eslint?
| max-width: 240px; | ||
| max-height: 40px; |
There was a problem hiding this comment.
240px is huge, is this correct?
There was a problem hiding this comment.
not sure about this one, looks like intended change for max width and height. @greg-in-a-box could you double confirm this one when you get back. ill leave this as it is for now.
There was a problem hiding this comment.
requested by design to increase the width and height to max the search bar
| @@ -0,0 +1 @@ | |||
| export {default} from './Header'; | |||
| } | ||
|
|
||
| if (typeof url === 'string') { | ||
| return <img alt="" className="be-logo-custom" src={url} />; |
There was a problem hiding this comment.
shouldn't we use an alt for accessibility?
src/elements/common/header/Logo.tsx
Outdated
|
|
||
| function getLogo(url?: string) { | ||
| if (url === 'box') { | ||
| return <BoxLogo width={45} height={25} />; |
There was a problem hiding this comment.
im surprised didn't complain about sorted props
| logoUrl?: string, | ||
| onSearch: Function, | ||
| onSearch: any, | ||
| searchQuery: string, |
There was a problem hiding this comment.
searchQuery isn't required prop anymore?
There was a problem hiding this comment.
searchQuery was used for input value prop. The searchInput has a self-controlled value state.
There was a problem hiding this comment.
can this be moved to the common/__mocks__ folder? needs to rebase first
There was a problem hiding this comment.
yes, gonna rebase and resolve conflicts for this.
tjiang-box
force-pushed
the
content-explorer-search
branch
from
f3b56a1 to
49b3c73
Compare
| @@ -1,2 +1 @@ | |||
| // @flow | |||
There was a problem hiding this comment.
flow file still needs this, no?
There was a problem hiding this comment.
still need it because of contentExplorer.js flow file
3 participants
Default Page
With Search Input entered
Box Logo
