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

TMS-1052: Favorite programs #182

Merged
merged 5 commits into from
Sep 3, 2024
Merged

TMS-1052: Favorite programs #182

merged 5 commits into from
Sep 3, 2024

Conversation

eebbi
Copy link
Contributor

@eebbi eebbi commented Aug 28, 2024

Severa-ID: 2132
Severa-kuvaus: TMS-1052 Suosikki-merkkaustoiminto
Task: https://hiondigital.atlassian.net/browse/TMS-1052

Description

Favorite program-functionalities

  • Favorites added and loaded dynamically with DustpressJS from LocalStorage
  • Added "Add to favorites" button to single-program page
  • Added favorite-listing dropdown-modal to header

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@eebbi eebbi requested a review from a team August 28, 2024 07:45
@HPiirainen HPiirainen self-assigned this Aug 28, 2024
Copy link
Contributor

@HPiirainen HPiirainen left a comment

Choose a reason for hiding this comment

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

Hyvä setti, vilkuile vielä huomiot

const $ = jQuery;

/**
* Class LoadMore
Copy link
Contributor

Choose a reason for hiding this comment

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

Väärä kommentti

@@ -0,0 +1,280 @@
/* eslint-disable no-console */
/**
* Load more functionality for koulutusnosto-component.
Copy link
Contributor

Choose a reason for hiding this comment

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

Väärä kommentti

* Class LoadMore
*/
export default class FavoritePrograms {
docReady() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Näille parille metodille vois heittää docblock-kommentin vielä

* Initialize functionalities common for all templates.
*/
initCommon() {
this.favoriteButtons = $( this.selectors.button );
Copy link
Contributor

Choose a reason for hiding this comment

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

Kaikki luokan variablejen asetukset kannattaisi olla tossa cache metodissa.

this.loadFavorites();
} );

$( document ).on( 'click', '.js-remove-favorite', ( e ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Voisiko tällä classilla tehdä myös jonkun this.removeFavoriteButtonsin ja tehdä event listenerin samalla tavalla kuin tossa yllä?

Copy link
Contributor Author

@eebbi eebbi Aug 28, 2024

Choose a reason for hiding this comment

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

Piti toteuttaa tälleen, koska .js-remove-favorite painikkeet tulee dynaamisesti listaukseen yksittäisen suosikki-itemin mukana

Comment on lines 43 to 65
foreach ( $program_ids as $key => $post_id ) {
// ID
$posts[ $key ]['ID'] = $post_id;

// Program title
$posts[ $key ]['title'] = \get_the_title( $post_id );

// Comma separated program-type terms
$term_obj_list = \get_the_terms( $post_id, 'program-type' );
$terms_string = join( ', ', wp_list_pluck( $term_obj_list, 'name' ) );
$posts[ $key ]['program_types'] = $terms_string;

// Dates
$posts[ $key ]['start_date'] = date( "d.m.Y", strtotime( \get_field( 'start_date', $post_id ) ) );
$posts[ $key ]['start_info'] = \get_field( 'start_info', $post_id );

// Permalink
$posts[ $key ]['permalink'] = \get_the_permalink( $post_id );

// Texts
$posts[ $key ]['remove_favorite_text'] = \_x( 'Remove from favorites', 'theme-frontend', 'tms-theme-tredu' );
$posts[ $key ]['go_to_program_text'] = \_x( 'Go to program', 'theme-frontend', 'tms-theme-tredu' );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tämä voisi olla myös vähän selkeämpi näin:

$term_obj_list = ...
$terms_string = ...

$posts[ $key ][] = [
    'ID' => $post_id,
    'title' => \get_the_title( $post_id ),
    // jne.
];

Toi $posts[ $key ]['jotain'] toisto on vähän turhaa.

<button id="js-favorites-open"
class="is-rounded button js-toggle is-outlined p-0 ml-2"
aria-controls="js-favorites-container"
aria-expanded="false"
Copy link
Contributor

Choose a reason for hiding this comment

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

Olin kattovinani, että tätä ei ikinä asetettaisi trueksi, vaikka nappia painaisi? Tän vois vielä varmistaa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tää tapahtuu jo valmiina olevassa toggle.js tiedostossa

<div class="button-container">
<a href="{permalink|url}" class="button is-primary">{go_to_program_text|html}</a>
<button
data-program-id={ID|attr}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data-program-id={ID|attr}
data-program-id="{ID|attr}"

@@ -0,0 +1,11 @@
<button data-program-id={program_id|attr}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<button data-program-id={program_id|attr}
<button data-program-id="{program_id|attr}"

Comment on lines +7 to +9
<svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 516 516">
<path fill="transparent" stroke="#042546" stroke-width="50" d="M47.6 300.4L228.3 469.1c7.5 7 17.4 10.9 27.7 10.9s20.2-3.9 27.7-10.9L464.4 300.4c30.4-28.3 47.6-68 47.6-109.5v-5.8c0-69.9-50.5-129.5-119.4-141C347 36.5 300.6 51.4 268 84L256 96 244 84c-32.6-32.6-79-47.5-124.6-39.9C50.5 55.6 0 115.2 0 185.1v5.8c0 41.5 17.2 81.2 47.6 109.5z"/>
</svg>
Copy link
Contributor

Choose a reason for hiding this comment

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

Tää olis parempi jos saisi ikoniksi ja sitä kautta includattua partiaalista

@eebbi eebbi merged commit 7016403 into master Sep 3, 2024
1 of 2 checks passed
@eebbi eebbi deleted the TMS-1052 branch September 3, 2024 05:29
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