dpc-sdp / ripple

Ripple is the frontend framework for Single Digital Presence, delivered using Nuxt and Vue.js
Apache License 2.0
61 stars 37 forks source link

[SDPAP-7477]remove call to function getExposedItemsToLoadField #1305

Closed yeniatencio closed 1 year ago

yeniatencio commented 1 year ago

Motivation and Context

This change is required to remove the items per page dropdown box where it gives the option to select the number of pages shown. This option is being added in BE content collection by selecting numbers of results shown. 3, 6, 9.

JIRA issue: https://digital-vic.atlassian.net/browse/SDPAP-7477

BE link: https://nginx-php.pr-272.content-reference-sdp-vic-gov-au.sdp4.sdp.vic.gov.au/ FE link: https://app.pr-1305.ripple.sdp4.sdp.vic.gov.au/

Changed

  1. Remove call to function getExposedItemsToLoadField.
  2. Remove required src in responsiveImage.vue because when selecting layout grid view and card display style Thumbnail in BE for the content collection component and if any of the results has no image then it will throw an error of Missing required prop: src . There is already a validation on the template
    
    <template>
    <img v-if="src" class="rpl-responsive-image" :class="`rpl-responsive-image--fit-${fit}`" :alt="alt" :height="height" :width="width" ref="image" :src="src" :srcset="calcSrcSet" :sizes="calcSizes" :style="calcFocalPoint"  />
    </template>


### Screenshots
![Screenshot 2023-06-05 at 4 21 34 pm](https://github.com/dpc-sdp/ripple/assets/47239456/7aebbefa-c5bb-4971-8319-6eca4702cad5)

<!--
Provide as many screenshots as required to make reviewers understand what was changed.
-->

## How Has This Been Tested?

<!-- All PR's should implement unit tests if possible -->
<!-- Please describe how you tested your changes. -->
<!-- Have you created new tests or updated existing ones? -->
<!-- e.g. unit | storybook | integration | none -->

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

- [ ] Bug fix (non-breaking change which fixes an issue)
- [X ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Improvement/refactoring (non-breaking change that doesn't add any features but makes things better)

## Checklist

<!-- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [ ] I've added relevant changes to the documentation.
- [ ] I have added tests to cover my changes (if not applicable, please state why)
- [ ] My change requires a template update for create-ripple-app.
- [ ] I have added template update script for next release.
alan-cole commented 1 year ago

Hi @dylankelly, @yeniatencio - I just noticed this PR in passing. The change here will break places where an exposed items per page is required, like on https://www.health.vic.gov.au/search?q=eye. If the purpose is to set the items per page, but without the user configurable option, then we shouldn't be using

$config['interface']['display']['options']['itemsToLoad']['values']

from here to set the value.

Instead, try using internal.itemsToLoad see schema:

$config['internal']['itemsToLoad']

This should be achievable in the back-end by updating the JSON configuration for the content collection, without needing this PR.

dylankelly commented 1 year ago

@yeniatencio In talking this through with @alan-cole , I don't think we should proceed with this PR. It looks like there has been some confusion here. The required change to add a defined number of results is needed for Ripple 2 only. By removing the ability to change the number of results we would break existing functionality in Ripple 1 (See link Alan provided).

@lambry we might need to pickup this ticket for Ripple 2.

Sorry for the confusion.

yeniatencio commented 1 year ago

@yeniatencio In talking this through with @alan-cole , I don't think we should proceed with this PR. It looks like there has been some confusion here. The required change to add a defined number of results is needed for Ripple 2 only. By removing the ability to change the number of results we would break existing functionality in Ripple 1 (See link Alan provided).

@lambry we might need to pickup this ticket for Ripple 2.

Sorry for the confusion.

Hi @dylankelly , I was going to close this pr too and Yes agree with you. I have already deleted the variable from this pr that I was using with my BE pr. Thanks guys.