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

Issue 15 basic app browsing #52

Merged
merged 4 commits into from
Nov 28, 2017
Merged

Issue 15 basic app browsing #52

merged 4 commits into from
Nov 28, 2017

Conversation

andrew-c-tran
Copy link
Contributor

The Problem/Issue/Bug:

a number of app browse screens (card layout) were redesigned in collaboration with @rickmanelius .

How this PR Solves The Problem:

layout was changed, relevant actions moved to dropdown, and clickable links created as per wireframe in #15

Manual Testing Instructions:

run make test to build the application and run the tests

Related Issue Link(s):

#15

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

  • I took this for a little spin, but didn't completely understand the point. I don't mind the new UI, but none of the features actually work do they? If this is intended to be just a visual change and not to be functional at all, please be more explicit about that in the PR. AFAICT none of the items in the dropdown do anything?
  • There is no hover on the dropdown items- users will expect that.
  • What does "browse" mean? I think that will have to be renamed.
  • What would "edit" mean?

screenshot_11_16_17__10_40_am

@andrew-c-tran
Copy link
Contributor Author

Pull updated to remove non-functional items from dropdown. default color hover added to menu (to be color matched to ddev scheme upon completion of style guide). loading spinner to be addressed in style guide update.

@andrew-c-tran
Copy link
Contributor Author

Rebased to include changes, tests pass, need another look at this to pull in

@rfay
Copy link
Member

rfay commented Nov 27, 2017

This doesn't work for me at all with ddev v0.10.0. Blank white screen, no sites shown even through 3 are running. Please, please add support for telling when something is going wrong. IMO that should be the first priority for ddev.

…cli that was lost in incorrect merge conflict resolution
@andrew-c-tran
Copy link
Contributor Author

The issue was corrected and I concur, error handling is long overdue at this point. Will shift gears after this PR and focus on #42

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

I guess I'm ok with this. It works now. Please always build and test your work when submitting a PR, or mark it as WIP.

I don't understand why every command in ddev-shell.js is not using js output at this point? Haven't we already stumbled on this twice or three times? Would it be appropriate to make that change here?

Is it ok that "Browse Local files" doesn't work?

@andrew-c-tran
Copy link
Contributor Author

at this point everything is over to json. The reason this happened here is that this was a long running PR and it exposed a weakness in our automated testing.

When the PR was opened, the tests all passed. When the describe PR was merged in, we shifted to using all json output from the CLI and that merge was rebased into this PR, and the tests again passed. The view layer, however, was not expecting the change of the json format to include httpurl and httpsurl and failed out. This is something that our integration tests that actually confirm the contract between CLI and what the app is expecting is still valid that we discussed would handle, and I think i'll tackle said tests that exercise the CLI right after the error handling story that's in flight now.

@andrew-c-tran andrew-c-tran reopened this Nov 28, 2017
@andrew-c-tran andrew-c-tran merged commit fcf1fd3 into ddev:master Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants