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

Add glossary #194

Merged
merged 16 commits into from
Oct 23, 2017
Merged

Add glossary #194

merged 16 commits into from
Oct 23, 2017

Conversation

hbillings
Copy link
Contributor

@hbillings hbillings commented Oct 10, 2017

Closes #115.

peek 2017-10-11 15-06

screenshot from 2017-10-11 15-42-10

To do:

  • Install the 18F glossary
  • Ensure the glossary only exists on jekyll pages
  • Style the glossary as a fly-out panel a la FEC.

@hbillings hbillings self-assigned this Oct 10, 2017
@hbillings hbillings changed the title Add glossary [WIP]Add glossary Oct 10, 2017
@adborden
Copy link
Contributor

@AvivaOskow @RyanSibley this is ready for your eyes. You can also edit the glossary terms by editing this branch.

@adborden adborden changed the title [WIP]Add glossary Add glossary Oct 11, 2017
@adborden adborden assigned adborden and unassigned hbillings Oct 11, 2017
@adborden
Copy link
Contributor

@hbillings this is ready for review.

}

// Wait for DOM to be loaded before initializing the glossary
document.addEventListener('DOMContentLoaded', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this something we could potentially fold back into the glossary library? It would be nice to be able to improve that. :-/

Copy link
Contributor

Choose a reason for hiding this comment

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

Mixed feelings... Generally I prefer libraries to not handle this kind of loading and leave it up to the app. I think it's strange that the glossary calls DOM APIs, because it makes it a bit more difficult to run in environments where the DOM is not available (like running tests in node). I would prefer to pass in DOM elements instead, which would allow the app to control when to re-initialize the glossary in case elements move around or something.

@@ -0,0 +1,108 @@
/* eslint-disable */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was kind of thinking this might be easiest to maintain as a markdown file that inherits the json.html jekyll layout -- what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooo yeah, that is smart.

@@ -0,0 +1,10 @@
<button class="js-glossary-toggle glossary-toggle">Glossary</button>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯 to breaking this out into an include

Copy link
Contributor

Choose a reason for hiding this comment

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

... you did that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...oh. Good for me?! 😂

position: fixed;
top: 0;
width: 75%;
z-index: $z-glossary;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, clever, I like that

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I'm not really familiar with the uswds scss files, so I'm not sure if they have their own set of variables for this kind of thing. I was surprised to see the z-index so high on the uswds nav.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't really have z-index variables, which I think is kind of surprising. I like being able to track it in one spot!

@hbillings
Copy link
Contributor Author

This is great! I'll let design approve it, but I'm good with it.

Copy link
Contributor

@RyanSibley RyanSibley left a comment

Choose a reason for hiding this comment

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

beautiful. does this mean I can edit now?

@adborden
Copy link
Contributor

adborden commented Oct 12, 2017 via email

@AvivaOskow
Copy link

@adborden looks good!I I haven't had a chance to play around with the styles yet, but I might want to change a couple small things if we have time design wise.

@adborden adborden merged commit 1c4e6db into master Oct 23, 2017
@adborden adborden deleted the add-glossary branch October 23, 2017 21:50
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

4 participants