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

V1.0.0 beta #44

Merged
merged 10 commits into from
Mar 18, 2017
Merged

V1.0.0 beta #44

merged 10 commits into from
Mar 18, 2017

Conversation

wad3g
Copy link
Contributor

@wad3g wad3g commented Mar 14, 2017

Navigation

Added basic navigation with several configuration options (#28).

Options

  • .nav-fixed: navigation will be fixed to the top of the page
  • .nav-right: navigation list (links) aligned to the right
  • .nav-center: navigation list (links) aligned center
Navigation Default

nav-default

Navigation Center

nav-center

Navigation Right

nav-right

Editor Config

Added .editorconfig to keep coding style consistent.

@kbrsh
Copy link
Owner

kbrsh commented Mar 16, 2017

Thanks for the PR! Love the idea of a nav. I made a couple of changes, making things more appealing, and using system fonts instead of Open Sans. Some files I think that added some extra bloated features were also removed.

Adding a navigation component would be cool, but I think there should be one base class for a navigation, and alignment shouldn't be built in. Why? Wing is meant to be as minimal as possible, using a minimal amount of classes, making it intuitive for developers.

Some improvements:

  • No background: Color schemes of sites may vary, and adding a fixed background, while easy to update, should be done by the developer manually.
  • If there will be alignment/configuration options, make them more self explanatory:
    • <div class="nav fixed"></div>, this could be targeted with something like: nav.fixed
  • Remove the color:black, and leave it to the developer to decide.

Also, if you could git pull on your side and update the PR, that would be great 👍

@wad3g
Copy link
Contributor Author

wad3g commented Mar 17, 2017

Yeah, I went back and forward between the nav options.

I'll take care of these and send another PR.

@@ -35,10 +35,26 @@
justify-content: center;
}

.right {
Copy link
Owner

Choose a reason for hiding this comment

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

Use something like .nav.right instead of .right, as it might interfere with other classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.right could have multiple uses though. it's the equivalent to .center

@wad3g
Copy link
Contributor Author

wad3g commented Mar 18, 2017

@kingpixil updated nav utility classes

@kbrsh
Copy link
Owner

kbrsh commented Mar 18, 2017

Sorry, I see what you mean. Do you think these should be added as utilities, or specifically for the navigation?

@wad3g
Copy link
Contributor Author

wad3g commented Mar 18, 2017

I can go either way.

Personally, nav specific classes for .nav-right / .nav-left is more intuitive and IMO, but at the same time is someone is building something with flexbox and wanted flexbox utility classes for say .right / .left the classes would essentially be the exact same.

@kbrsh
Copy link
Owner

kbrsh commented Mar 18, 2017

Cool, I'd say let's kill two birds with one stone, add .right/.left, and they can be used for the navigation, and as a utility.

@wad3g
Copy link
Contributor Author

wad3g commented Mar 18, 2017

Done. Reverted the utility classes back to their original generic .right / .left from .nav.right / .nav.left.

I'm going to hop on your gitter chat - I've got a few questions whenever you get a minute.

@kbrsh
Copy link
Owner

kbrsh commented Mar 18, 2017

Looking good! I'll be on Gitter for any questions, thanks for the PR! 🙌

@kbrsh kbrsh merged commit 5db445d into kbrsh:v1.0.0-beta Mar 18, 2017
@kbrsh kbrsh mentioned this pull request Mar 18, 2017
kbrsh added a commit that referenced this pull request Jun 17, 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