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

Custom labels: update search and DataView #1077

Merged
merged 17 commits into from Oct 10, 2019

Conversation

@AquiGorka
Copy link
Contributor

commented Sep 17, 2019

Requires aragon/aragon-ui#615


@AquiGorka AquiGorka requested review from bpierre and sohkai Sep 17, 2019
@AquiGorka AquiGorka referenced this pull request Sep 17, 2019
17 of 64 tasks complete
@bpierre

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

@AquiGorka Just looking at the gif, the button and the text field don’t seem to have the same margin, is it intended?

@AquiGorka AquiGorka changed the base branch from feature/custom-labels-dataview to master Sep 18, 2019
@AquiGorka AquiGorka force-pushed the feature/mobile-search-full-width branch from 013593e to f2f6e7b Sep 18, 2019
@AquiGorka

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

@bpierre good eye, from a quick research the inconsistency seems to come from the heading wrapper in DataView it seems to set the side paddings to 24px (3gu) while the designs have it at 16px (2gu), then, the full width search input uses absolute positioning and sets the side paddings at the correct value.
Ideas on how to proceed?
I'll open another issue to track this issue with the DataView component.

@AquiGorka

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

Margin has been fixed here

sohkai added 2 commits Oct 9, 2019
`}
>
{compact ? (
<TextInput

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 9, 2019

Member

Seems weird we'd need to define our own custom input if SearchInput was available?

Could we make SearchInput work in the compact version too?

This comment has been minimized.

Copy link
@bpierre
bpierre added 3 commits Oct 9, 2019
@sohkai sohkai changed the title Custom labels mobile search full width Custom labels: update search and DataView Oct 10, 2019
[value, theme]
)
useEffect(() => {
if (mode && searchInputRef.current) {

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 10, 2019

Member

Should this be mode === SEARCH_OPEN?

This comment has been minimized.

Copy link
@bpierre

bpierre Oct 10, 2019

Member

Yes 🙈

@now now bot temporarily deployed to staging Oct 10, 2019 Inactive
@now now bot temporarily deployed to staging Oct 10, 2019 Inactive
@sohkai
sohkai approved these changes Oct 10, 2019
Copy link
Member

left a comment

LGTM!

@bpierre bpierre merged commit 18d9b18 into master Oct 10, 2019
3 of 6 checks passed
3 of 6 checks passed
License Compliance FOSSA is analyzing this commit
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
now Deployment has completed
Details
@bpierre bpierre deleted the feature/mobile-search-full-width branch Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.