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

BiG-CZ: WDC / CUAHSI Details View #2259

Merged
merged 6 commits into from
Sep 15, 2017
Merged

Conversation

rajadain
Copy link
Member

Overview

Adds specific items to CUAHSI's details view, based on comments in #2234 (comment) and this Google doc. Important to note: this does not fetch the detail information for a given item. That will be done in #2238, which depends on #2243.

Also tagging @aufdenkampe for visual review.

Connects #2234

Demo

image

Notes

Some fields have placeholder values, such as the citation in the above screenshot. These placeholders seem arbitrary, and to convert them all into real values may be a long-tailed effort. Currently they are left as is.

Testing Instructions

  • Check out this branch. Run bundle
  • Go to :8000/?bigcz
  • Search for something and open the WDC tab. Click on any item to get to the Details view.
  • Ensure the values match the description in the Google doc and the accompanying source comment.

@caseycesari
Copy link
Member

Links in the source tooltips are not clickable because any mouse event on the tooltip closes it. We've dealt with this somewhere before in the app, but I remember it getting a little messy.

image

<p>
<!-- TODO Make This Work -->
<a class="zoom" href="#">
<i class="fa fa-search-plus"></i> Zoom to extent
Copy link
Member

Choose a reason for hiding this comment

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

I thought that all WDC results were point-based. If that's the case, it may not make sense to have a "zoom to extent" feature.

@rajadain
Copy link
Member Author

Links in the source tooltips

Also, that description comes in from the API as plain text. We don't know it has links. We'll have to use a regex or something to transform them into links.

@caseycesari
Copy link
Member

Also, that description comes in from the API as plain text. We don't know it has links. We'll have to use a regex or something to transform them into links.

Good point. It may make sense to let it be as is for now.

@ajrobbins
Copy link

ajrobbins commented Sep 14, 2017

Links in the source tooltips are not clickable

We definitely addressed this elsewhere in the app, such as the boundary selection tooltips:
screen shot 2017-09-14 at 5 56 02 pm

We should replicate that here; all tooltips should be potentially clickable, even if they don't have links at the moment.

@mmcfarland
Copy link
Contributor

We definitely addressed this elsewhere in the app

The difference here is that we're not providing the content for the popup. They're indicating that we'd have to parse for things that appear to be links and convert them from plain text.

@ajrobbins
Copy link

ajrobbins commented Sep 15, 2017

The difference here is that we're not providing the content for the popup. They're indicating that we'd have to parse for things that appear to be links and convert them from plain text.

I don't think we have to convert them, but even to support people copy-pasting a plain text link, they need to be able to click on the popup.

@caseycesari
Copy link
Member

Tooltip stuff sounds like a good follow-up card.

@rajadain
Copy link
Member Author

Context menus were not captured in this GIF, but as can be seen, the latest fixup makes it so that the text is selectable, copyable, and pasteable making those URLs a little more accessible:

2017-09-15 10 43 16

@caseycesari
Copy link
Member

Some fields have placeholder values, such as the citation in the above screenshot. These placeholders seem arbitrary, and to convert them all into real values may be a long-tailed effort. Currently they are left as is.

Just to clarify, the citation values coming back from the server sometimes have placeholder values, like in the PR screenshot?

@rajadain
Copy link
Member Author

Yeah, like this:

Citation: U.S. Geological Survey, [YEAR], National Water Information System data, accessed [DATE ACCESSED] via HIS Central (http://hiscentral.cuahsi.org).

Copy link
Member

@caseycesari caseycesari left a comment

Choose a reason for hiding this comment

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

Looks good! Nice job splitting up the commits into related chunks of work.

from rest_framework.serializers import CharField, DateTimeField
from rest_framework.serializers import (CharField,
DateTimeField,
ListField
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but consider adding a trailing comma here.

@caseycesari caseycesari assigned rajadain and unassigned caseycesari Sep 15, 2017
 * Rename `activeCatalog` -> `catalog`, since for a result detail
   there is only one catalog, so the additional qualifier is waste.
 * Rename `cuahsiSearchResult` -> `searchResultCuahsi` so when the
   files are sorted, it shows up near `searchResult` which is the
   default implementation.
These will allow us to do really catalog specific things
in each detail view.
Previously we were joining list values into strings before
sending them to the client. This dictated client presentation,
and made it difficult to treat the data differently.

To allow clients to decide how best to show these lists, they
are now sent back as lists. The client template is updated to
maintain current behavior.
These values will be used in the front-end.
 * Title now features location, which is extracted from the id
 * Source has service_org and service_title, with the description
   inside a popup
 * Instead of the calendar icon, we now use the text "Data
   collected on" or "Data collected between" based on if the end
   date is different from the beginning date
 * Sample mediums are listed
 * Source Data button is shown if details_url exists
 * Web Services button is shown pointing to the service url
 * Last collected value is shown in relative time, using a new
   toTimeAgo filter
 * A table of concept keywords is added
   - This currently only features the keyword name
   - Values and units will be pulled in the future, see #2238
 * There is comment placeholder for adding charts, coming in #2238
 * Citation in the bottom
 * Some style edits to make it all fit

The popover() and bootstrapTable() functions will only execute if
the template has any corresponding data-toggle elements. Thus,
they noop for CINERGI and HydroShare.
Previously, with 'focus', the popover would disappear after a
click. With 'click', it stays around until explicitly closed,
either by the close button within the popover, or by re-clicking
the info icon that opened it. This allows the user to select
text in the popover, and in the case of URLs, paste them into
new tabs to visit the relevant sites.
@rajadain
Copy link
Member Author

Thanks for the review! Squashed all the fixups, added that last trailing comma, will merge now.

@rajadain rajadain merged commit c490839 into develop Sep 15, 2017
@rajadain rajadain deleted the tt/bigcz-cuahsi-detail-view branch September 15, 2017 15:51
@aufdenkampe
Copy link
Member

aufdenkampe commented Sep 19, 2017

This all looks great. Did you all figure out that the text after "Citation:" comes directly from the "citation" field in the response from a GetServices request, which should be cached for all the services? See the Sample_WDC_Site_Detail_BiGCZPortal_SearchResult Google Doc that I set up as a mockup and map to web service response fields.

@rajadain
Copy link
Member Author

Yes, those values are coming from the (soon to be cached, see #2260) Services request, as specified in the Google Doc. Thanks for setting that up, it made the implementation considerably more straightforward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants