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

Handle missing values in the preview #115

Closed
petrpyszko opened this issue Oct 23, 2018 · 25 comments
Closed

Handle missing values in the preview #115

petrpyszko opened this issue Oct 23, 2018 · 25 comments
Assignees
Labels
bug groomed The issue has been groomed and should be in a good shape. hacktoberfest help wanted refactoring up-for-grabs

Comments

@petrpyszko
Copy link

petrpyszko commented Oct 23, 2018

Motivation

The app crashes unexpectedly in the preview mode for articles without the image or products without the price. Preview Delivery API returns items in the draft step and some values can be missing (even required fields).

Proposed solution

  • The app should handle missing values and display some placeholder without data or with some explanation (e.g,. "Price TBA")
  • check other possible occurences of this problem

Additional context

image

@pavan-krishna-v
Copy link

@petrsvihlik .. I see this is a new issue. Can i take it up. Or if you have already existing priority issues do let me know ! Thanks :)

@petrsvihlik
Copy link
Contributor

Hi @pavan-krishna-v, you can certainly pick it up! I've just sent you an invitation to the project. I'll be able to assign the issue to you once you accept it.

@pavan-krishna-v
Copy link

Hi @pavan-krishna-v, you can certainly pick it up! I've just sent you an invitation to the project. I'll be able to assign the issue to you once you accept it.

Done. Thanks !

@petrsvihlik
Copy link
Contributor

Assigned. Let us know if you need anything :)

@pavan-krishna-v
Copy link

Motivation

The app crashes unexpectedly in the preview mode for articles without the image or products without the price. Preview Delivery API returns items in the draft step and some values can be missing (even required fields).

Proposed solution

  • The app should handle missing values and display some placeholder without data or with some explanation (e.g,. "Price TBA")
  • check other possible occurences of this problem

Additional context

image

Hi @petrpyszko ,

I understand that the issue is beyond just unavailablity of price. Also, i see that both coffee products are failing to load and both brewery pages are working. All the products have the price.

The url which i checked is, http://localhost:3000/en-us/store/brewers.

And i notice that all article pages are failing on clicking of preview.

Do let me know if i got the issue right.

@petrpyszko
Copy link
Author

Hi @pavan-krishna-v ,

yes, you are right, the App should be working even for the new (completely blank) items (brewers, articles ....). I am not sure which other fields can cause similar issues - but after creating blank items of each content type, you should be able to quickly identify other problematic areas :)

@Simply007
Copy link
Contributor

Hi @pavan-krishna-v,

since this is a sample application, it is completely fine to have just a show case, how to implement missing values. So it will be OK to protect only articles or products and not to implement all the cases where we could face missing values.

Take a look to the solution in our Vue sample app for inspiration.

Hope it helps 😄

@Simply007
Copy link
Contributor

Hello @pavan-krishna-v ,

do you have any update on this issue?

@Karl-EdwardFPJeanMehu
Copy link
Contributor

Karl-EdwardFPJeanMehu commented Jan 27, 2019

If efforts to solve this issue have been abandoned, I'd like to take it. However, I don't see the issue for this page. Please provide information on how to reproduce error.

@petrsvihlik
Copy link
Contributor

Hi @Karl-EdwardFPJeanMehu , I've just sent you an invitation to the project. I'll be able to assign the issue to you once you accept it.

You should be able to reproduce the issue if you turn on the preview functionality as described at https://github.com/Kentico/cloud-sample-app-react#previewing-content-from-your-project

You can generate a sample project at https://app.kenticocloud.com/sample-project-generator and turn on the preview functionality as described at https://developer.kenticocloud.com/docs/previewing-content-in-a-separate-environment#section-using-the-delivery-preview-api

@Simply007
Copy link
Contributor

Hi @Karl-EdwardFPJeanMehu,

in the related task (basically the same but in Vue application), there is a description how to reproduce the error step by step).

But instead of npm run serve, you would use npm start and connection to the client is described here:

@Karl-EdwardFPJeanMehu
Copy link
Contributor

Thanks @Simply007 ! I'll check it out

@Simply007
Copy link
Contributor

If you have any question, feel free to ask!

If you want to assign this issue to you, just accept the invitation @petrsvihlik has sent you:
https://github.com/Kentico/cloud-sample-app-react/invitations

@Karl-EdwardFPJeanMehu
Copy link
Contributor

Karl-EdwardFPJeanMehu commented Jan 31, 2019

I had found the issue but just took some time to make sure. The metaData state variable is initially undefined. That is causing the typeError in the MetaData comp for all given prop values (ie metadataMetaTitle). I would like to know is there a set of preferred default values I may consult to set them to?

@Simply007
Copy link
Contributor

Simply007 commented Jan 31, 2019

I would use the same default values that are in Vue sample app at least for Article:
https://github.com/Kentico/cloud-sample-app-vue/pull/19/files#diff-48bf359bb9d62bda17a5805ab100d8d1

The other values are not strictly defined, so would use the same pattern:

- No <fieldname> filled
- (<contenttype> has not <element name>)

@Karl-EdwardFPJeanMehu
Copy link
Contributor

Thanks!

@Karl-EdwardFPJeanMehu
Copy link
Contributor

Karl-EdwardFPJeanMehu commented Feb 4, 2019

What should I use as a default image? I'm currently using a simple svg image with the text "product has no image" at its center. Otherwise, simply returning (<contenttype> has not <element name>) as you mentioned above will return an invalid image source path and so a broken image will be rendered.

@Simply007
Copy link
Contributor

In a Vue.js there is an image omited.

I would be more for the placeholder div with the text: <contenttype> has not <element name> with the same size as the image to not corupt the layout.

@Karl-EdwardFPJeanMehu
Copy link
Contributor

Ok, I see it.

@Karl-EdwardFPJeanMehu
Copy link
Contributor

Nearly complete. Please advise for meta OG images. These can only be replaced by a default image.

@Simply007
Copy link
Contributor

I assume you mean the OG image that is passed to the Metadata component.
It is fine like it is implemented right now - when you pass the null to the component then the meta tag is not rendered.

@Karl-EdwardFPJeanMehu
Copy link
Contributor

Yes, the implementation as you described is ok but I've tested it and the app throws an error when those SSO tags are left empty in the app.

@Simply007
Copy link
Contributor

I will take a look at the #125 pull request and let you know!

@Simply007
Copy link
Contributor

I have gone through the pull request and everything works fine.

According to the metadata (i.e. ogImage) I would recommend using the lodash for the value check:

import _ from 'lodash';
...
      {!_.has(props, 'ogImage.value[0].url') ? null : ( // {!props.ogImage ? null : (
        <meta property="og:image" content={props.ogImage.value[0].url} />
      )}

But I am completely fine to accept the pull request without it.
So it depends on you whether you want to implement it!

Great jop @Karl-EdwardFPJeanMehu, thanks!

@Karl-EdwardFPJeanMehu
Copy link
Contributor

Thanks @Simply007 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug groomed The issue has been groomed and should be in a good shape. hacktoberfest help wanted refactoring up-for-grabs
Projects
None yet
Development

No branches or pull requests

5 participants