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

Move create new button #3622

Merged
merged 1 commit into from
Jun 24, 2020
Merged

Move create new button #3622

merged 1 commit into from
Jun 24, 2020

Conversation

LukasHirt
Copy link
Collaborator

Adjusted also a bit the spacing around table header.

image

@LukasHirt LukasHirt requested a review from kulmann June 17, 2020 08:34
@LukasHirt LukasHirt self-assigned this Jun 17, 2020
@LukasHirt
Copy link
Collaborator Author

Hmm, not sure why tests are failing in CI. Locally are passing fine and there is no other change then different width of breadcrumbs 🤷

@LukasHirt LukasHirt force-pushed the move-create-button branch from d4055ab to 6e9fb3f Compare June 17, 2020 10:51
@LukasHirt
Copy link
Collaborator Author

I tried rebasing the PR. Let's see.

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

👍

@LukasHirt
Copy link
Collaborator Author

I haven't figured out what is happening with the tests. Locally cannot reproduce the errors because it all passes just fine. And I don't get that only changing width of an element can fail two separate tests. 🤷

@LukasHirt LukasHirt force-pushed the move-create-button branch from 6e9fb3f to 98c3435 Compare June 22, 2020 08:11
@individual-it
Copy link
Member

the tests don't really click on the "Upload File" button but click the "+ New" button, check that the "Upload File" button exists and then upload the file with "setValue" it happens that the dropdown overlays the modal dialog
image

in https://github.com/owncloud/phoenix/blob/533e8cd9e1451a4d27778f1ef51ba7aa271d2dc4/tests/acceptance/pageObjects/filesPage.js#L261 the test click on the modal dialog to close the dropdown, but because of the overlay it clicks "New mark-down file..." and that app seems to be broken and redirects to an empty window

So we could:

  1. click not on 0,0 but e.g. 100,100 - but how to make sure that a similar issue hits us again when the dropdown gets longer?
  2. close the dropdown directly in selectFileForUpload() again
  3. don't open the dropdown at all

@kulmann
Copy link
Member

kulmann commented Jun 23, 2020

IMO we should close the dropdown by doing another click on the + New button, as it toggles the dropdown.

@LukasHirt LukasHirt force-pushed the move-create-button branch from 1242a4e to f2236da Compare June 23, 2020 09:57
@LukasHirt
Copy link
Collaborator Author

I've moved the click to the actual dropdown. Since the dropdown has padding, we do not click on any link inside and are also safe in the future in case it would be growing in size. Pls, check if you're fine with this solution @individual-it @kulmann

IMO we should close the dropdown by doing another click on the + New button, as it toggles the dropdown.

I've tried this solution. Using click() directly on that button doesn't work at all. And I am a bit afraid of clicking at the location of the button because I am not 100 % sure it wouldn't get covered in case the dropdown would be way expanded too much.

Copy link
Member

@individual-it individual-it left a comment

Choose a reason for hiding this comment

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

LGTM

We have moved the create new button from the right side of files app bar
to the left directly next to breadcrumbs.
@LukasHirt LukasHirt force-pushed the move-create-button branch from f2236da to 3ca616c Compare June 24, 2020 07:35
@kulmann
Copy link
Member

kulmann commented Jun 24, 2020

Yes, awesome!

@LukasHirt LukasHirt merged commit e5c1c71 into master Jun 24, 2020
@LukasHirt LukasHirt deleted the move-create-button branch June 24, 2020 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants