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

[feature]: ErrorView #2936

Merged
merged 20 commits into from
Jan 14, 2021
Merged

[feature]: ErrorView #2936

merged 20 commits into from
Jan 14, 2021

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Jan 11, 2021

Description

Goal: A generic error view component that handles some basic styling. Should take props allowing for customization, and provide defaults.

Also in this PR:

  • Cleanup of unused components
  • Refactor of ErrorView and NotFound into single component.
  • Fixed some Storybook issues, namely global styles being applied and fonts not being imported.

Related Issue

Closes PWA-1032.

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Check each of the updated page types and ensure functionality in non-error state.
  2. On each of the page types, cause error to be thrown. Simplest way is to block specific data GQL request for that component. Observe that the error view is shown as expected, for that page. Some may have custom messages, others will show default messages.
  3. Go to a non-existent route ie https://pr-2936.pwa-venia.com/foo. It should render the error view.

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have added translations for new strings, if necessary.
  • I have updated the documentation accordingly, if necessary.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jan 11, 2021

Messages
📖

Associated JIRA tickets: PWA-1032.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against ff34cea

@sirugh sirugh force-pushed the rugh/pwa-1032-notFoundPage branch from 3244994 to 7559ac4 Compare January 12, 2021 18:26
@schensley
Copy link

@sirugh thanks for the work on this. UX approved.

@sirugh sirugh marked this pull request as ready for review January 12, 2021 20:38
@sirugh sirugh added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Jan 13, 2021
revanth0212
revanth0212 previously approved these changes Jan 13, 2021
Copy link
Contributor

@revanth0212 revanth0212 left a comment

Choose a reason for hiding this comment

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

Nice work buddy. Much needed feature.

@supernova-at
Copy link
Contributor

QA

The UseCmsPage talon does return an error when the GraphQL query fails, but the CMS component itself never looks at that error when determining how to render.

As a result, if the cmsPage query fails, the homepage forever shows the page loading indicator instead of this error message.

Should this logic be fixed in this PR?

@sirugh
Copy link
Contributor Author

sirugh commented Jan 14, 2021

As a result, if the cmsPage query fails, the homepage forever shows the page loading indicator instead of this error message.

This is now remedied by changing shouldShowLoadingIndicator to be loading && !data, so it only shows when the loading boolean from the query hook is true. Once something is returned by the network call we either render the content (hasContent) or we pass rendering responsibility to the child, categoryList component, which will now render the error view if necessary.

I opted not to handle the error state for cmsPage as that's a little out of scope of this work.

Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
revanth0212
revanth0212 previously approved these changes Jan 14, 2021
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
@dpatil-magento dpatil-magento merged commit 6d32f84 into develop Jan 14, 2021
@dpatil-magento dpatil-magento deleted the rugh/pwa-1032-notFoundPage branch January 14, 2021 23:15
@sirugh sirugh restored the rugh/pwa-1032-notFoundPage branch April 26, 2021 16:26
@sirugh sirugh deleted the rugh/pwa-1032-notFoundPage branch April 26, 2021 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:extensions pkg:peregrine pkg:venia-concept pkg:venia-ui Progress: done version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants