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 1092 fix #1145

Merged
merged 7 commits into from
Mar 7, 2017
Merged

Issue 1092 fix #1145

merged 7 commits into from
Mar 7, 2017

Conversation

yoshi2095
Copy link
Contributor

@yoshi2095 yoshi2095 commented Feb 20, 2017

Proposed changes in this pull request

Changed the order of divs (contact vs projects) on org pages - fix for issue 1092.

Functionality

  • Are all requirements met? Compare implemented functionality with the requirements specification.
  • Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

  • Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.

@CLAassistant
Copy link

CLAassistant commented Feb 20, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@oliverroick oliverroick left a comment

Choose a reason for hiding this comment

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

Hi @yoshi2095, thank you for your contribution.

There a few issues which I would like to ask you to resolve before we can proceed with your PR.

In general, the issue is fixed; the project list is displayed before organisation details. However, your changes introduce some other problems:

Between a screen width of 991px and 1625px the right-hand panel containing organisation and contacts informations is almost not visible.

screen shot 2017-02-21 at 17 28 06

In mobile view (screen width < 991px) the panel containing organisation and contacts informations isn't visible as well. The projects list in this view should also span the complete width of the screen and not just ~2/3.

screen shot 2017-02-21 at 17 28 16

Could you please have a look into these issues? Once fixed, we can proceed and pass your PR on to a second reviewer to have a closer look at your code.

Thanks!

@clash99
Copy link
Contributor

clash99 commented Feb 21, 2017

Hi @yoshi2095 -

You only should need to adjust the css for responsive max-width: 991px.

The display: table and display: table-header-group properties are allowing the divs to stack opposite of their order in the html. If you undo those styles, it should fix the issue. I hope that helps!

Thanks,
Chandra

@yoshi2095
Copy link
Contributor Author

Hi @oliverroick and @clash99, thanks for the review. I'll just commit the suggested changes. Thanks.

@yoshi2095
Copy link
Contributor Author

yoshi2095 commented Feb 22, 2017

Hi @oliverroick and @clash99, I have committed the new changes. Now, It's working just the way it should be. Everything is responsive, all the issues that @oliverroick highlighted in the previous commit are now solved and, the projects list appear before the contact information.

Copy link
Member

@oliverroick oliverroick left a comment

Choose a reason for hiding this comment

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

Hi @yoshi2095, there are still many issues with your PR.

In Chrome, the right-hand panel is still not visible:

screen shot 2017-02-22 at 17 38 01

In Firefox, there's a gap at the end of the panel, which shouldn't be there:

screen shot 2017-02-22 at 17 39 51

In Firefox, when the user is logged out the panel doesn't span the whole height of the page:

screen shot 2017-02-22 at 17 41 45

In Safari, a similar gap is there but on top of the panel:

screen shot 2017-02-22 at 17 41 01

Please make sure the pages look consistent across all browsers.

Many thanks!

@oliverroick
Copy link
Member

@yoshi2095 I also just noticed you have been changing the CSS in main.css directly. main.css is a file which is auto-generated by SASS. You need to change the code in any of the *.scss files in that directory to make sure your changes persist.

@yoshi2095
Copy link
Contributor Author

yoshi2095 commented Feb 22, 2017

@oliverroick Hi oliver, thanks for the review. Sure, I will soon make the changes as suggested by you. But one thing, I can't reproduce the first case. In Chrome, the right-hand panel is still not visible. In my tab, everything is visible just the right way:

screenshot-3

Although, I was able to produce the errors in other browsers, ill just fix them very soon. I'll make sure that I make the changes that work well in all the browsers. Thanks again.

@yoshi2095
Copy link
Contributor Author

yoshi2095 commented Feb 23, 2017

Hi @oliverroick, fixed the issue. I have rechecked everything from my side, for every browser.

Copy link
Member

@oliverroick oliverroick left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good. @clash99 can you check if the HTML and CSS are ok?

@oliverroick oliverroick requested a review from clash99 February 23, 2017 16:15
@yoshi2095
Copy link
Contributor Author

yoshi2095 commented Mar 3, 2017

hi @oliverroick, please confirm if its done or are there any other changes needed? Thanks.

Copy link
Contributor

@clash99 clash99 left a comment

Choose a reason for hiding this comment

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

Can you remove the display: table-footer-group completely for #organization-single .detail? I don't believe it's needed and removing it will improve the spacing. Let me know if you have questions. Thanks!

@yoshi2095
Copy link
Contributor Author

@clash99 made the suggested changes. Please review. Thanks.

@yoshi2095
Copy link
Contributor Author

Hi @clash99, Please confirm if its done or are there are any other changes needed. Thanks.

@clash99
Copy link
Contributor

clash99 commented Mar 6, 2017

Looks good - thanks @yoshi2095

@amplifi
Copy link
Contributor

amplifi commented Mar 6, 2017

@yoshi2095 Thanks for completing this PR! If you rebase your branch onto our master, I can merge your code.

@yoshi2095
Copy link
Contributor Author

@amplifi done :)

@amplifi amplifi merged commit 9e1ca6a into Cadasta:master Mar 7, 2017
laura-barluzzi pushed a commit to laura-barluzzi/cadasta-platform that referenced this pull request Mar 14, 2017
* issue-1092-fixed

* issue-1092-fix-changes-made

* issue-1092-fix

* issue-1092-fix-minor change

* issue-1092-finally-fixed

* fixed-issue-1092

* removed table-header-group-property
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.

5 participants