search

OpenEnergyPlatform / oeplatform

Repository for the code of the Open Energy Platform (OEP) website. The OEP provides an interface to the Open Energy Family
http://openenergyplatform.org/
GNU Affero General Public License v3.0
61 stars 19 forks source link

Implemented new Welcome site mockup from Bryan #458

Closed johannwagner closed 4 years ago

Bachibouzouk commented 4 years ago

@johannwagner - instead of writing [WIP] in the title you could have chosen to create a Draft Pull Request

Bachibouzouk commented 4 years ago

@johannwagner - could we merge this? What is left to do in this branch?

Git meta-comment I would suggest that you make several conceptual commits with specific messages in the future (like add link for ..., create layout for ..., worked css for ..., ect.), rather than one large commit with a message essentially being the title of the PR. Think of a PR as a report with a sequence of several conceptual paragraphs (the commits), help the reviewer to understand what you did and which steps you followed :)

It might seem a bit more efforts, but it helps the code being better reviewed and thus the overall quality of the code we push is improved :)

Bachibouzouk commented 4 years ago

@johannwagner - don't forget to implement the design idea of @4lm, as this was approved by @bmlancien here

The proposed template structure is like this :

image

The actual template structure is like this :

image

@johannwagner let me or @4lm know if you need help to either retrieve the commits from #447 or modify the structure of the template

johannwagner commented 4 years ago

I implemented the changes from #447 into my branch. I was not able to cherry-pick this, so I did a best guess of changes and it works quite good.

Bachibouzouk commented 4 years ago

@johannwagner - I don't see a footer tag in the html code, it is intentionnal? The header and footer are simply divs with navbar class.

@bmlancien is it standard and can it pose problems for reactivness?

bmlancien commented 4 years ago

@Bachibouzouk Omitting the HTML5 tags such as footer has no influence on responsiveness, but it influences positively accessibility, SEO and makes the website standards-compliant.

Bachibouzouk commented 4 years ago

@Bachibouzouk Omitting the HTML5 tags such as footer has no influence on responsiveness, but it influences positively accessibility, SEO and makes the website standards-compliant.

So if I understand correctly, one should not use the footer tag @bmlancien ? Can you provide a link with definition of what SEO is? And a reference for the standard?

johannwagner commented 4 years ago

@Bachibouzouk I added a footer, however the nav tag should be good enough for screen readers.

bmlancien commented 4 years ago

@Bachibouzouk Omitting the HTML5 tags such as footer has no influence on responsiveness, but it influences positively accessibility, SEO and makes the website standards-compliant.

So if I understand correctly, one should not use the footer tag @bmlancien ? Can you provide a link with definition of what SEO is? And a reference for the standard?

@Bachibouzouk Sorry, I didn't express myself correctly. Having the tag is a better practice (but not a must)!

SEO = Search Engine Optimization

And about HTML5 tags

4lm commented 4 years ago

@johannwagner thanks for this PR, 50 changed files, a lot of work I assume! I have a few remarks, if they are solved we are IMO good to go:

  1. In the console I get 3 errors that 3 icon fonts are not found.
  2. Undefined by mockup: All buttons have a flat design, but the login button has not, should be IMO flat as well.
  3. Undefined by mockup: The login button in desktop view is on the left side of the link list in the navbar. Should IMO on the right side of the link list (more common this way).
  4. In mobile view the spacing of the login button is not vertically aligned to the hamburger button. Also the spacing towards navbar bottom and between hamburger button and login button is a little off.
  5. The OEP logo is not vertically aligned in the navbar (desktop view).
  6. In comparision to the mockup, headings in body ("Open Energy Platform", "Tutorials") must be in a darker blue.
  7. Heading "Tutorials" is missing the hat icon to its left.
  8. All buttons in the mockup are more rounded.
  9. The vertical alignment of the "Tutorial" section in the body is compared to the mockup a little off.
  10. The button "See Tutorials" is dark grey, but should be dark blue. Undefined by mockup: the hover state should IMO also be blue, either dark blue background with white font or light blue background.
  11. In the mobile view the tutorial section is stacked too close to the next section. The "See Tutorials" button leans right against the next section, not a single pixel padding or margin.
  12. Footer in the mobile view should not be vertically stacked, please compare with mockup.
  13. In the mobile view the app has horizontal overflow with horizontal scrolling. This should not be the case.
  14. Undefined by mockup: IMO at least in the mobile view the navbar should be sticky.
Bachibouzouk commented 4 years ago

@johannwagner thanks for this PR, 50 changed files, a lot of work I assume! I have a few remarks, if they are solved we are IMO good to go:

Thanks @4lm for you careful review, I opened an issue with your suggestions as this PR is already quite full in terms of file changed. Let's address your suggestions in a subsequent PR :)

4lm commented 4 years ago

Ok, just talked to @Bachibouzouk, will proceed like he suggested. I will merge this PR and we address my suggestions in a subsequent PR/Issue #503.