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.

Sign up for GitHub

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

Jump to bottom

Media Section: Style cleanup for action bar #11873

Merged
merged 5 commits into from
Mar 9, 2017
Merged

Media Section: Style cleanup for action bar #11873

merged 5 commits into from
Mar 9, 2017

Conversation

iamtakashi
Copy link
Contributor

Fixes #11834

Bottom part of each screenshot is how it'd look with this PR.

Section (mobile)
mobile

Section (desktop)
large

Modal (mobile)
mobile-2

Modal (desktop)
large-2

@iamtakashi iamtakashi requested review from retrofox and gwwar March 8, 2017 01:25
@matticbot matticbot added the [Size] S Small sized issue label Mar 8, 2017
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=update/media-action-bar

@retrofox retrofox force-pushed the try/media-section-v2 branch from 66fce78 to 2474f28 Compare March 8, 2017 17:02
@retrofox
Copy link
Contributor

retrofox commented Mar 8, 2017 •
edited
Loading

This branch should be targeted to try/media-section-v2 @iamtakashi
it's already targeting to try/media-section-v2. The problem was it needed a rebase.

@retrofox retrofox force-pushed the try/media-section-v2 branch from 5d54642 to 98cdd8e Compare March 8, 2017 21:34
@retrofox retrofox force-pushed the update/media-action-bar branch from e9b37f9 to a2fa081 Compare March 8, 2017 23:34
@matticbot matticbot added the [Size] S Small sized issue label Mar 8, 2017
@retrofox
Copy link
Contributor

retrofox commented Mar 8, 2017

tested width: 580px

should we concern with these dimensions?

@gwwar gwwar added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 8, 2017
@retrofox retrofox added the [Feature] Media The media screen in Calypso, general media management, or integration with third party media. label Mar 8, 2017
@gwwar
Copy link
Contributor

gwwar commented Mar 8, 2017

Padding looks tight with the add url control open:
screen shot 2017-03-08 at 3 51 50 pm

@retrofox
Copy link
Contributor

retrofox commented Mar 8, 2017

Padding looks tight with the add url control open:

@gwwar let me rebase again. We've already been working on this section.

@retrofox retrofox force-pushed the update/media-action-bar branch from a2fa081 to 963b9e7 Compare March 8, 2017 23:55
@matticbot matticbot added the [Size] S Small sized issue label Mar 8, 2017
@retrofox
Copy link
Contributor

retrofox commented Mar 8, 2017

After the rebase it produces a flickr switching to upload-by-URL

flickr

@gwwar
Copy link
Contributor

gwwar commented Mar 9, 2017

We're getting there 😄 but this needs more polish.

I'm not seeing the flicker, but we could use some more padding in both sections. There's also some shadow going on in the modal:

screen shot 2017-03-08 at 4 06 23 pm

screen shot 2017-03-08 at 4 07 17 pm

Error States look a bit off too:

screen shot 2017-03-08 at 4 07 48 pm

screen shot 2017-03-08 at 4 08 14 pm

@retrofox
Copy link
Contributor

retrofox commented Mar 9, 2017 •
edited
Loading

There's also some shadow going on in the modal:

#11932 ;-)

@iamtakashi
Copy link
Contributor Author

Good catch! I'll fix them shortly.

@retrofox
Copy link
Contributor

retrofox commented Mar 9, 2017 •
edited
Loading

@iamtakashi , just FYI the issues about some shadow going on in the modal as fixed here: #11932. You shouldn't concern about this, but maybe you should update your branch.

@matticbot matticbot added [Size] M Medium sized issue and removed [Size] S Small sized issue labels Mar 9, 2017
@iamtakashi
Copy link
Contributor Author

It was quite painful to work with but it now should be good!

@retrofox retrofox force-pushed the update/media-action-bar branch from 55cbf7f to fb5ed73 Compare March 9, 2017 16:59
@matticbot matticbot added the [Size] M Medium sized issue label Mar 9, 2017
@iamtakashi iamtakashi removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 9, 2017
@iamtakashi
Copy link
Contributor Author

This isn't ready for review yet, it needs further work since #11932 is merged.

@retrofox retrofox force-pushed the try/media-section-v2 branch from 9ae39f5 to 2e280ee Compare March 9, 2017 18:55
@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] M Medium sized issue labels Mar 9, 2017
@retrofox retrofox force-pushed the update/media-action-bar branch from 7d0a5a4 to 34570ab Compare March 9, 2017 19:04
@matticbot matticbot added [Size] M Medium sized issue and removed [Size] XL Probably needs to be broken down into multiple smaller issues labels Mar 9, 2017
@gwwar gwwar added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Mar 9, 2017
@matticbot matticbot added the [Size] M Medium sized issue label Mar 9, 2017
@gwwar
Copy link
Contributor

gwwar commented Mar 9, 2017

Noticed the that mobile views could use a bit of ❤️

screen shot 2017-03-09 at 11 42 10 am

@gwwar
Copy link
Contributor

gwwar commented Mar 9, 2017

Let's get this PR in and address some of the other issues in future PRs

@retrofox
Copy link
Contributor

retrofox commented Mar 9, 2017

Thanks a lot @iamtakashi. Let me merge this one for you. 👍

@retrofox retrofox merged this pull request into try/media-section-v2 Mar 9, 2017
@retrofox retrofox deleted the update/media-action-bar branch March 9, 2017 20:07
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 9, 2017
@retrofox retrofox mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gwwar gwwar gwwar approved these changes

@retrofox retrofox Awaiting requested review from retrofox

Assignees
No one assigned
Labels
[Feature] Media The media screen in Calypso, general media management, or integration with third party media. [Size] M Medium sized issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@iamtakashi @matticbot @retrofox @gwwar