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.

Sign up for GitHub

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

Jump to bottom

Add 'Ask GIS' panel and reorganize SCSS #115

Merged
merged 2 commits into from
Feb 13, 2024
Merged

Add 'Ask GIS' panel and reorganize SCSS #115

merged 2 commits into from
Feb 13, 2024

Conversation

jazairi
Copy link
Contributor

@jazairi jazairi commented Feb 9, 2024 •
edited by JPrevost
Loading

The main purpose of this PR is to add 'Ask GIS' links to all views except the index, as specified in GDT-129. This is toggled by a feature flag so it does not appear in other TIMDEX UI apps. (However, a similar feature may be useful for those apps, too.)

Secondarily, this removes some rules from timdexui.scss. This is unrelated to the ticket, but I noticed a comment in that file suggesting that those rules should be relocated, and this seemed like a good opportunity to do so. Check the commit message for details, as two rulesets were removed altogether.

Developer

  • All new ENV is documented in README
  • All new ENV has been added to Heroku Pipeline, Staging and Prod
  • ANDI or Wave has been run in accordance to
    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)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Requires database migrations?

NO

Includes new or updated dependencies?

NO

@mitlib mitlib temporarily deployed to timdex-ui-pi-gdt-129-as-2osqqy February 9, 2024 19:17 Inactive
@jazairi
Reorganize rules in timdexui.scss
911752f 
Why these changes are being introduced:

There are some CSS rules in `timdexui.scss` with a note that
they should be moved out of a file if there's a better place to
put them. Since then, we've added several partials that might make
logical sense to contain to these rules.

Relevant ticket(s):

N/A.

How this addresses that need:

This makes the following changes to `timdexui.scss`:

* The `#advanced-search-panel legend` rule moves to the `panels`
partial. (Though, it would also make sense in the `search` partial.)
* The `.top-space` rule moves to the `results` partial, as it is only
used for the filter container spacing.
* The `.top-attached` and `.top-line` rules are removed. Currently,
`.top-line` is only used in the style guide layout, and
`.top-attached` appears not to be used anywhere.

Side effects of this change:

The style guide layout will no longer have a top border.
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-129-as-2osqqy February 9, 2024 20:12 Inactive
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-129-as-2osqqy February 9, 2024 20:25 Inactive
@coveralls
Copy link

coveralls commented Feb 9, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 7892605935

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.444%
Totals Coverage Status
Change from base Build 7849916842: 0.0%
Covered Lines: 358
Relevant Lines: 360
💛 - Coveralls

@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-129-as-2osqqy February 9, 2024 20:49 Inactive
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-129-as-2osqqy February 9, 2024 20:50 Inactive
@jazairi
Copy link
Contributor Author

jazairi commented Feb 9, 2024

Sanity check request for the reviewer: try running the tests locally and let me know if you see that weird behavior I shared in Slack. It seems no longer to be a problem for me with a fresh copy of the repo, for whatever reason.

JPrevost
JPrevost reviewed Feb 9, 2024
View reviewed changes
app/views/record/_sidebar.html.erb
@@ -8,11 +8,5 @@
</div>
<% end %>

<div class="bit askus">
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we losing this default contact logic the way this is implemented?

I wonder if we could continue down the path you started here and have a single shared/ask partial and within that partial split out the gdt/non-gdt logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that makes sense. The original 'Ask Us' bit felt kind of placeholder-y, but that's not my assumption to make.

@JPrevost
Copy link
Member

JPrevost commented Feb 9, 2024

@jazairi tests worked as expected for me locally

@mitlib mitlib temporarily deployed to timdex-ui-pi-gdt-129-as-nmitw8 February 13, 2024 17:05 Inactive
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-129-as-nmitw8 February 13, 2024 17:57 Inactive
@jazairi
Copy link
Contributor Author

jazairi commented Feb 13, 2024

@JPrevost Sorry I missed your review on this. I just pushed a commit that I think addresses your feedback. To minimize complexity, I removed the p tag in the non-GDT ask us bit and normalized the titles for both. Personally, I prefer the more minimal design that Darcy proposed and figure it should apply to all TIMDEX UI apps, but I don't mind walking that back if you think changing non-GDT stuff is out of scope here.

@jazairi jazairi requested a review from JPrevost February 13, 2024 18:37
JPrevost
JPrevost reviewed Feb 13, 2024
View reviewed changes
app/views/record/_sidebar.html.erb Outdated
<a class="btn button-secondary" href="https://libraries.mit.edu/ask/">Ask Us</a>
</p>
</div>
<%= render 'shared/ask_gis' %>
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename this partial to shared/ask now that it is handling both use cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

D'oh. Yes.

@jazairi jazairi requested a review from JPrevost February 13, 2024 20:18
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-129-as-nmitw8 February 13, 2024 20:18 Inactive
@jazairi
Add 'Ask GIS' panel
07a35ba 
Why these changes are being introduced:

We want to provide users multiple paths to request help from the
GIS team.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/GDT-129

How this addresses that need:

This adds an 'Ask GIS' link to all views except the index:

* On the results page with reuslts, in the filter sidebar below
the filters.
* On the results page with no results, in a sidebar to the right.
* On the full record page, in the sidebar below fact panels.

There is a minor difference in placement for the results view in
smaller viewports, which is accommodated with media queries.

Side effects of this change:

The new `ask` partial contains both the logic for the existing
'Ask Us' link and the 'Ask GIS' link. In order to minimize the
complexity, both use the same markup. This removes the descriptive
paragraph for 'Ask Us' and changes the header from 'Ask Us' to
'Need help?', both of which seem like usability improvements.
@jazairi jazairi temporarily deployed to timdex-ui-pi-gdt-129-as-nmitw8 February 13, 2024 20:53 Inactive
@jazairi jazairi merged commit fe1fa4b into main Feb 13, 2024
5 checks passed
@jazairi jazairi deleted the gdt-129-ask-gis branch February 13, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JPrevost JPrevost JPrevost approved these changes

@matt-bernhardt matt-bernhardt Awaiting requested review from matt-bernhardt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jazairi @coveralls @JPrevost @mitlib