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

Updated Toolbar Styling #16

Merged
merged 6 commits into from
Apr 26, 2017

Conversation

CaitW
Copy link
Member

@CaitW CaitW commented Apr 25, 2017

screen shot 2017-04-25 at 11 10 03 am

Additional styling suggestions welcome.

  • The new style is constrained slightly by the fixed height of the header (75px, and therefore toolbar: 25px).
  • The buttons have hover affordances (underlines, pointer-cursors) to indicate their selectability.
  • Individual icons are specified via CSS
  • Having to specify individual icons in the CSS is messy:
.tool-measure {
    .icon {
        .fa; // extend font-awesome to include generic icon styles
            &:before {
                content: @fa-var-arrows-h; // specify particular icon to be used 
            }
        }
    }
}

@theduckylittle
Copy link
Member

Starting to get there! Please add the following:

  1. Convert tabs to spaces.
  2. Place the "#header" change into test.css and not in the less file as it's not a part of the toolbar.
  3. Also remove the code for "#toolbar" from test.css. There should be no ID referenced CSS in the LESS files.

The goal of the CSS generated by LESS is not to do layout work but to provide styling for the individual components. How the toolbar is placed, rendered, and sized is up to the application but not up to GeoMoose.

@theduckylittle
Copy link
Member

One last thing! As this is your first PR please add your name to CONTRIBUTORS.md (you may need to pull in upstream/master to get the file).

Everything else looks great!

@theduckylittle theduckylittle merged commit c8ac502 into geomoose:master Apr 26, 2017
@CaitW CaitW deleted the default-toolbar-styling branch April 26, 2017 13:31
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