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

Fixes #50: Improve organization of “chart gallery” #52

Merged
merged 14 commits into from
Mar 15, 2024

Conversation

maggiejaenicke
Copy link
Contributor

Changes made:

  • Adds carousels for quarterly river conditions and monthly flow tiles.

    • I decided to add river conditions videos directly into the site for a few reasons. The drupal link seems to error a lot, and I think maybe we consider replacing that slide click trigger (handleSlickClick() function on line 57 of RiverConditions_Carousel.vue) to drupal to go to twitter instead (or removing it altogether and just having the videos as they are in the site). When I had the carousel as just images, I could not get the thumbnails to stop opening in full screen without disabling full screen for other elements throughout the app which is why I pivoted to embedded videos.

Testing:

Before making this pull request, I:

  • Cleaned the code the way Vue likes it - run 'npm run lint --fix'
  • Made sure all tests run
  • Ran WAVE plugin 508 compliance tool

I can confirm this has been checked on:

  • Chrome
  • Safari
  • Edge
  • Firefox
  • Samsung Internet
  • Internet Explorer 11 (not supported, but still needs at least a working user redirect page)

Jaenicke, Margaret (Contractor) Ellen added 13 commits January 30, 2024 10:35
…art and initiates new files for RiverConditions_Carousel
…s, cleans up riverConditionsCharts object array, adjusts carousel to use river conditions charts for slider images
…utlines to both flow and river conditions charts
…in the site, removes those little boxes on Recent, Vis, and About in the header
…ds in more videos to the js file, updates gray border styling for river conditions
…r both flow and river conditions, adds link to river conditions code
@cnell-usgs cnell-usgs self-requested a review March 11, 2024 20:55
Copy link
Member

@cnell-usgs cnell-usgs left a comment

Choose a reason for hiding this comment

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

Nice job, this worked great for me:
image

One thing I noticed that is cool, but may also slow the page down, is that each of the videos for river conditions can be played in the site. That's pretty nice. As there are more and more we will need to be smart about using lazy loading so that page doesn't load all of the videos at start. Similarly, the chart gallery charts use full size images for the thumbnails and load all charts when the carousel is clicked in. In a future improvement we should implement lazy loading to optimize the page. Please add an issue to note this, otherwise looks good to merge in.

@@ -146,6 +146,16 @@ $coolBlue: rgb(66, 145, 235);

}
}
.usa-nav__primary button[aria-expanded=false] span::after { //removes those little squares that were appearing by Recent, Visualizations, About
Copy link
Member

Choose a reason for hiding this comment

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

🔥

display: inline-block;
height: 0px;
width: 0px;
content: "";
Copy link
Member

Choose a reason for hiding this comment

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

Curious - what does this content: "" do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Looks like nothing. Removed.

src/components/PortfolioAccordions.vue Outdated Show resolved Hide resolved
src/components/PortfolioAccordions.vue Show resolved Hide resolved
@maggiejaenicke
Copy link
Contributor Author

@cnell-usgs Made a few changes and opened issues based on your comments, so it should be good to merge in! I can't merge it myself because of my permissions on this repo.

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