-
Notifications
You must be signed in to change notification settings - Fork 0
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
Allow for optional platform names in header #58
Conversation
Pull Request Test Coverage Report for Build 7821033145
💛 - Coveralls |
FYI, to the reviewer: the easiest way to test this is to pull this branch to your local theme gem repo, point your Gemfile to that path and branch in the local git repo of a Rails app of your choice, then add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the use case this is envisioning, this looks like it should work fine, so I think this is fine to merge as is:
However, I think we're going to need to consider the mobile / small screen behavior of this feature in order to extend it to systems like ETD, or any tool that is going to need more than a few letters.
That can probably be something that happens at the style library level, though. We'd need to update this gem again when that happens anyway, so removing the styles added here in favor of those we'll inherit shouldn't be something we forget to do.
If that makes sense to you, then I'm happy to now.
OH - one other thing. I wanted to ask about whether the new environment variable needed to be documented anywhere? |
@matt-bernhardt Oh yeah, good point about mobile viewports. I think I'll add some preliminary SCSS here, so when we do move this upstream, there will be something to work with. Thanks! As for documentation, I plan to add this as an optional variable to each Rails repo's readme. That's how we've documented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that looks good - thanks for laying this foundation for when this moves up to the style library!
Why these changes are being introduced: Matt added the platform name to Omeka to remind users where in the MITL ecosystem they are. We were planning to add this to Geodata when we realized that it would probably be useful in other Rails apps, and should thus go in the theme gem. Relevant ticket(s): https://mitlibraries.atlassian.net/browse/GDT-124 How this addresses that need: This adds an optional platform name in the header, based on the `PLATFORM_NAME` environment variable. Environments that don't set `PLATFORM_NAME` will continue to have the standard header. Side effects of this change: The SCSS updates for this should likely be moved upstream to the style guide, but I couldn't decide if that should happen before or after this merges.
d162398
to
54b822e
Compare
Why these changes are being introduced:
Matt added the platform name to Omeka to remind users where in the MITL ecosystem they are. We were planning to add this to Geodata when we realized that it would probably be useful in other Rails apps, and should thus go in the theme gem.
Relevant ticket(s):
https://mitlibraries.atlassian.net/browse/GDT-124
How this addresses that need:
This adds an optional platform name in the header, based on the
PLATFORM_NAME
environment variable. Environments that don't setPLATFORM_NAME
will continue to have the standard header.Side effects of this change:
The SCSS updates for this should likely be moved upstream to the style guide, but I couldn't decide if that should happen before or after this merges.
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO