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-950: Contact & place-of-business sync fuctionalities #162

Merged
merged 11 commits into from
Nov 7, 2023
Merged

Conversation

eebbi
Copy link
Contributor

@eebbi eebbi commented Oct 27, 2023

Task: https://geniemdev.atlassian.net/browse/TMS-950
Severa-ID: 2132

Description

Add functionalities for new tms-plugin-contact-sync & tms-plugin-place-of-business-sync plugins

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 October 27, 2023 09:26
@HPiirainen HPiirainen self-assigned this Oct 27, 2023
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.

Kts. huomiot

flex: 1 0 auto;

@include from($widescreen) {
max-width: calc(25% - 0.75rem);
Copy link
Contributor

Choose a reason for hiding this comment

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

Tähän voisi tehdä jonkun tiedostokohtaisen SCSS-variablen, esim. $contacts-gap: .75rem, niin olisi helpompi muokata sitä tarvittaessa. Mutta ei pakko muuttaa. Pitäisi olla myös .75rem eikä 0.75rem.

Copy link
Contributor

Choose a reason for hiding this comment

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

CSS gridillä saisi myös tehtyä vähän nätimmin näitä sarakkeisiin jaettuja näkymiä.

assets/styles/blocks/custom/_contacts.scss Show resolved Hide resolved
Comment on lines 115 to 133
->set_key( "{$key}_place_of_business_post" )
->set_name( 'place_of_business_post' )
->set_filters( [ 'search' ] )
->redipress_include_search( function ( $places ) {
if ( empty( $places ) ) {
return '';
}

$results = [];

foreach ( $places as $place_id ) {
$results[] = \get_field( 'title', $place_id );
}

return implode( ' ', $results );
} )
->set_post_types( [ 'placeofbusiness-cpt' ] )
->set_return_format( 'id' )
->set_instructions( $strings['place_of_business_post']['instructions'] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Sisennys kuntoon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eikös nuo oo jo kunnossa, kun ovat redipress_include_search -funktion sisällä? 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

Koko putkesta siis kyse.

$place_of_business_post_field = ( new Field\Relationship( $strings['place_of_business_post']['label'] ) )
->set_key( "{$key}_place_of_business_post" )

-->

$place_of_business_post_field = ( new Field\Relationship( $strings['place_of_business_post']['label'] ) )
    ->set_key( "{$key}_place_of_business_post" )

lib/ACF/Fields/PlaceOfBusinessFields.php Show resolved Hide resolved
@@ -85,7 +85,7 @@ public function format( array $data ) {
$filled_api_contacts ?? []
);

$data['column_class'] = 'is-10-mobile is-offset-1-mobile is-6-tablet is-offset-0-tablet is-4-desktop';
$data['column_class'] = '';
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änhän vois jättää kokonaan pois jos laitetaan vaan tyhjä stringi

'menu_order' => 'ASC',
'meta_value' => 'ASC', // phpcs:ignore
],
] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Olisko parempi laittaa tuo uusi kenttä palauttamaan suoraan koko Post-objekti eikä pelkkää ID:tä? Vaikuttaa vähän turhalta tää query, kun datan saisi suoraan kentästä.


return array_map( function ( $id ) {

foreach( \get_field_objects($id) as $field ) {
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
foreach( \get_field_objects($id) as $field ) {
foreach( \get_field_objects( $id ) as $field ) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Eikö pelkkä \get_fields() toimisi, niin ei tarvi tehdä omaa luuppia ja saisi kaiken yhdellä kertaa?

$args = [
'post_type' => Contact::SLUG,
'posts_per_page' => 200, // phpcs:ignore
'post_status' => 'publish',
'fields' => 'ids',
'post__in' => array_map( 'absint', $selected_contacts ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Saisiko tässäkin kentän mieluummin palauttamaan suoraan ne objektit, niin ei tarvitsisi tehdä uutta queryä?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mitäs meinaat näillä query-kommenteilla? Ei jostain syystä nyt aukee yhtään mulle 😄

@@ -393,7 +393,7 @@ public function menu_item_classes( $classes, $item ) : array {
}

$current_page = \get_queried_object();
if ( (int) $item->object_id === $current_page->ID ) {
if ( isset( $current_page->ID ) && (int) $item->object_id === $current_page->ID ) {
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
if ( isset( $current_page->ID ) && (int) $item->object_id === $current_page->ID ) {
if ( ! empty( $current_page->ID ) && (int) $item->object_id === $current_page->ID ) {

@@ -58,9 +58,9 @@
{>"ui/icon" icon="building" class="icon--large is-primary" /}
</div>

<div>
<p class="m-0">
Copy link
Contributor

Choose a reason for hiding this comment

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

Vähän hassu että tehdään <p> tagi jolta poistetaan marginit, voisi olla myös ihan vaan <span> niin ei tule automaattisesti turhia tyylejä 💁‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<span> ei itessään ole "aito" tekstielementti, ja semantiikan näkökulmasta tekisi saman kun aiemmin tuossa olleet tekstiä ympäröivät <div> elementit

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.

Tsekkaa vielä noi pari kohtaa


return array_map( function ( $id ) {

foreach( \get_field_objects( $id ) as $field ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tarviiko hakea koko objektit vai toimisko vaan return \get_fields( $id ); jos halutaan kaikki kentät hakea?

'meta_key' => 'last_name',
'orderby' => [
'menu_order' => 'ASC',
'meta_value' => 'ASC', // phpcs:ignore
],
];

$s = get_query_var( self::SEARCH_QUERY_VAR, false );
if ( ! \is_admin() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tätä modelia ei ajeta ikinä adminissa, joten is_admin tarkistus taitaa olla turha.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tän koko metodin vois tehdä selkeämmin näin:

$selected_contacts = \get_field( 'contacts' );
if ( empty( $selected_contacts ) ) {
    return [];
}

$s = \get_query_var( self::SEARCH_QUERY_VAR, false );

// Return all selected contacts if no search was performed.
if ( empty( $s ) ) {
     return $selected_contacts;
}

$args = [
    // ... kaikki query argsit
    's' => $s,
];

$the_query = new \WP_Query( $args );

return $the_query->posts;

Eli tehdään early return, jos hakuparametriä ei ole asetettu.

@@ -1,4 +1,4 @@
<div class="contacts__item column {column_class|attr} has-text-small keep-vertical-spacing">
Copy link
Contributor

Choose a reason for hiding this comment

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

Eihän joku muu olemassaoleva partial includaa tätä contact partialia ja aseta column_classia? Jos ei, niin sit ok poistaa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eip

@eebbi eebbi requested a review from HPiirainen November 2, 2023 10:45
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.

👌

@eebbi eebbi merged commit 2bd96fa into master Nov 7, 2023
1 check passed
@eebbi eebbi deleted the TMS-950 branch November 7, 2023 17:35
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.

3 participants