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

[WIP] Navigation #35

Merged
merged 18 commits into from Oct 22, 2017
Merged

[WIP] Navigation #35

merged 18 commits into from Oct 22, 2017

Conversation

Gushsd
Copy link
Contributor

@Gushsd Gushsd commented Jul 5, 2017

Updated header's links according to flexboxgrid framework.
Development of a dropdown menu project.

@Gushsd
Copy link
Contributor Author

Gushsd commented Jul 26, 2017

New changes of layout in Dropdown-Submenu under 'Community' and 'About' (essentially in the classes .submenu and .dropdown)

padding-top: 0.25em;
margin-top: 10px;
padding: 0px 10px;
padding: 0px 10px;
Copy link
Member

Choose a reason for hiding this comment

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

For consistency we should probably stick with one formatting option - either 0px or 0. Both are valid although I would tend for the later as the unit is irrelevant for this value. What do you think? 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Steffan, thanks for the comment. Yes, I agree. The unity then doesn't make any difference. We then stick to just '0' from now on!

dropdownOpen : function() {
$('.dropdown').on('click', 'a', function(event){
var target = event.target
if($(target).parent().hasClass('open')){
Copy link
Member

Choose a reason for hiding this comment

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

Instead of checking the class state yourself, how about using the .toggleClass(className) method of jQuery? It might be easier to read and is also results in less code to maintain 😄

@Gushsd
Copy link
Contributor Author

Gushsd commented Aug 22, 2017

We further improved features and styling in the submenu through the classes .dropdown and .submenu. Icons from FontAwesome were added (dropdown arrow and close sign; the 3 social links).

Copy link
Member

@gsambrotta gsambrotta left a comment

Choose a reason for hiding this comment

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

Generally looks good!
I still have to see how it look like but i start to write down some code improvments!
Let me know if anything is not clear 😃

url: /about/drive
- title: "Contact"
url: /about/contact
Copy link
Member

Choose a reason for hiding this comment

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

can you insert a new empty line here please? Thanks!


{% endif %}

{% endif %}
{% endfor %}
Copy link
Member

Choose a reason for hiding this comment

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

Also here, can we insert a new line?
I think this file also need a nice formatting. Can you please check that the indentation is done with 2 spaces and no 4 tabs? And that there are no random empty lines or weird indentation. Thanks :)

@@ -15,6 +15,7 @@

<link rel="stylesheet" href="{{ "/css/main.css" | prepend: site.baseurl }}">
<link rel="canonical" href="{{ page.url | replace:'index.html','' | prepend: site.baseurl | prepend: site.url }}">
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.7.0/css/font-awesome.min.css">
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this line before we call our main.css file.
In this way our css will be called after the library one.
This means our styles will be always the last one so will be the one that actually is applied. (so if we want to override some libraries style, we can do it ;) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

<ul class="main row col-xs-9 between-xs">
{% include _render_nav.html nav=site.data.nav.main %}
</ul>
<div class="main row between-xs col-xs-6">
Copy link
Member

Choose a reason for hiding this comment

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

Did we check how this look like in mobile? Does it need some adjustment or is good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm.. It looks... under construction.. With the 'toggle device' tool you can only have a little notion of the final effect I think... There is no way we can run the test page on a real smartphone? Can I generate the link some how so I could open in another device than the computer rendering the site through Jekyll? I doubt it...
I'll play with it a bit more and see if it should be changed..

a {
color: white;
background-color: $brand-color;
padding: 8px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(funny now, commenting myself.. kind of schizophrenic) I just realized that this line is breaking the alignment between the parent links in the menu. So I have to figure out a way of changing this padding-style change without breaking the original alignment.... Anyone has any ideas?

@gsambrotta
Copy link
Member

gsambrotta commented Sep 12, 2017

Hey @Gushsd i check the branch, we are getting there :D
I leave some comments here that we need to work on:
schermata 2017-09-12 alle 19 40 55

  • social icons needs to open in a new lin k (check target='_blank')
  • text is not vertically align with social icons
  • font should be little bigger
  • arrow/cross little bit more to the right
  • donate button should be pinky and with border like in the design
  • is just me or the header background is blu-his instead of white?

Let's keep this design mockup as guideline:
https://www.figma.com/proto/MHyUqUq6gd5EuGuIBerw2Z76/OTS-latest-design?scaling=min-zoom&redirected=1&node-id=2%3A2

I personally think you can change those stuff listed above, by your self. You know how to do it :)

About mobile, i forgot that we need to do the mobile/hamburger menu but we can have a look it together.


dropdownOpen : function() {
$('.dropdown').on('click', 'a', function(event){
var target = event.target
Copy link
Member

Choose a reason for hiding this comment

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

Are we going to use JavaScript Standard Style? Looks like some Semicolons are missing or omitted on purpose. Guess its better to stick with inserting them after a statement to ensure consistency 😉

@gsambrotta gsambrotta merged commit fc2a821 into master Oct 22, 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.

None yet

3 participants