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

Media section/add link #11672

Merged
merged 6 commits into from
Mar 6, 2017
Merged

Media section/add link #11672

merged 6 commits into from
Mar 6, 2017

Conversation

artpi
Copy link
Contributor

@artpi artpi commented Mar 1, 2017

Resolves #9135 , adds "Add" button next to media section in sidbar

zrzut ekranu 2017-03-01 o 15 16 27

CC @iamtakashi @gwwar @lamosty

@artpi artpi added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 1, 2017
@artpi artpi self-assigned this Mar 1, 2017
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] S Small sized issue label Mar 1, 2017
@iamtakashi
Copy link
Contributor

I was excited to see this and works great with Chrome, but it doesn't seem to work for me with Firefox and IE.

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Good catch @iamtakashi. It also looks like the
screen shot 2017-03-01 at 12 19 40 pm

button doesn't work in the section and modal for FF either.

@@ -86,6 +87,7 @@ const PublishMenu = React.createClass( {
queryable: true,
config: 'manage/media',
link: '/media',
buttonLink: '/media/' + site.slug,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's guard here, since site might not be available yet.

buttonLink: site ? `/media/${ site.slug }` : `/media`;

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, and it looks like testing http://calypso.localhost:3000/media/:siteId: with a clean cache comes up with a number of errors:
screen shot 2017-03-01 at 12 12 49 pm

To clean out the cache on chrome or ff:

localStorage.clear(); indexedDB.deleteDatabase( 'calypso' );

Copy link
Contributor

Choose a reason for hiding this comment

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

buttonLink: `/media/${ site.slug || '' }`;

?

@@ -73,6 +81,7 @@ module.exports = React.createClass( {
accept={ this.getInputAccept() }
multiple
onChange={ this.uploadFiles }
onClick={ this.onClick }
Copy link
Contributor

@retrofox retrofox Mar 2, 2017

Choose a reason for hiding this comment

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

We should rename this method according to what the method does. One line above there is a nice example. When the onChange event happens, then uploadFiles.
event => action to take.
Could it be something like openMediaModal?

@retrofox
Copy link
Contributor

retrofox commented Mar 2, 2017

The hover styles of the add button are missed there:

  • No blue-light color
  • Hand pointer instead of arrow (I'm not able to capture the pointer)

@matticbot matticbot added [Size] M Medium sized issue and removed [Size] S Small sized issue labels Mar 3, 2017
@jhnstn
Copy link
Member

jhnstn commented Mar 3, 2017

Just tested in FF, everything works as expected.
I cleared the cache and didn't see any errors in the console.

LGTM

className="media-library__upload-button-input" />
</form>
</Button>
<form ref="form" className={ classes }>
Copy link
Contributor

@gwwar gwwar Mar 3, 2017

Choose a reason for hiding this comment

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

Interesting, I take it that the form wasn't fully filling the button area. In prod, if you happen to browser zoom in on IE, then click the bottom right you'll see that nothing happens, but if you click on top left it triggers normally.

If we run into this again later, we can just make the font size absurdly huge on the hidden input.

See http://jsfiddle.net/gwwar/nFLKU/

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Verified that upload works on Chrome, FF, Safari and IE11/Edge. Also verified that no errors are thrown on a clean cache. Unless you had any other comments @iamtakashi I think this is good to 🚢

@iamtakashi
Copy link
Contributor

👍 Good to go!! This is so cool to see working.

@artpi artpi force-pushed the media-section/add-link branch from 1709c81 to 50d910f Compare March 6, 2017 16:36
@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] M Medium sized issue labels Mar 6, 2017
@artpi artpi force-pushed the try/media-section-v2 branch from 7791f0f to a343b88 Compare March 6, 2017 16:37
@artpi artpi merged commit 1a01b88 into try/media-section-v2 Mar 6, 2017
@artpi artpi deleted the media-section/add-link branch March 6, 2017 16:48
@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 6, 2017
retrofox pushed a commit that referenced this pull request Mar 8, 2017
* Add proper button for upload

* Make it look like other buttons

* Make upload button work in FF

* Add protection for when site is not ready yet

* Small cleanup

* Fixing css
retrofox pushed a commit that referenced this pull request Mar 8, 2017
* Add proper button for upload

* Make it look like other buttons

* Make upload button work in FF

* Add protection for when site is not ready yet

* Small cleanup

* Fixing css
retrofox pushed a commit that referenced this pull request Mar 9, 2017
* Add proper button for upload

* Make it look like other buttons

* Make upload button work in FF

* Add protection for when site is not ready yet

* Small cleanup

* Fixing css
retrofox pushed a commit that referenced this pull request Mar 9, 2017
* Add proper button for upload

* Make it look like other buttons

* Make upload button work in FF

* Add protection for when site is not ready yet

* Small cleanup

* Fixing css
retrofox pushed a commit that referenced this pull request Mar 10, 2017
* Add proper button for upload

* Make it look like other buttons

* Make upload button work in FF

* Add protection for when site is not ready yet

* Small cleanup

* Fixing css
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Size] XL Probably needs to be broken down into multiple smaller issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants