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

New gurb validation pages #105

Merged
merged 39 commits into from
Nov 21, 2024
Merged

New gurb validation pages #105

merged 39 commits into from
Nov 21, 2024

Conversation

JoanaFigueira
Copy link
Contributor

@JoanaFigueira JoanaFigueira commented Oct 21, 2024

Description

Validation pages for non-members

Changes

  • Add feature flag for gurb
  • Add all validation pages for non-members following a new design
  • Add address validation via API

Checklist

Justify any unchecked point:

  • Changed code is covered by tests.
  • Relevant changes are explained in the "Unreleased" section of the CHANGES.md file.
  • That section includes "Upgrade notes" with any config, dependency or deploy tweek needed on development and server setups.
  • Changes on the setup process (development, testing, production) have been updated in the proper documentation

Observations

Address validation will be addressed in a different PR

Please, review

  • the design is correct
  • CUPS validation works
  • When you start webforms-ui Gurb's related data is updated accordingly.
  • The address is properly filled

How to check the new features

  • npm start
  • make sure you have the backend running (webforms api)

Deploy notes

  • For now, the feature flag isGurbEnabled should be set to false until the gurb form is ready for deploy

Copy link

vercel bot commented Oct 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
webforms-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 21, 2024 10:50am

index.html Outdated
<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
<link href="https://fonts.googleapis.com/css2?family=Manrope:[email protected]&display=swap" rel="stylesheet">

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, the font's files (googleapis, gstatic) are imported 3 times, and I don't know it's necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, not anymore. At the first, there were 3 different fonts, but now there is only one, and we completely forgot to remove the unused ones. Thanks!!

)
}

export default GurbLoadingContext
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Gurb management is simple enough to put everything in one context file. (it's just my opinion, this way works fine for me too)

@@ -0,0 +1,48 @@
import axios from 'axios'

const WEBFORMS_API_URL = document.getElementById('root')?.dataset?.webformsApiUrl
Copy link
Contributor

Choose a reason for hiding this comment

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

Great!! To have an api file for each section is the best way to work!

</Box>
)
}
export default TextRecomendation
Copy link
Contributor

Choose a reason for hiding this comment

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

The structure looks fine to me, but I think there are some components that could be reused from the generic components that are there now. For example the Next and Previous buttons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new buttons (Next and Previous) are a bit different from what we have now... color, behavior, even icons. This is why we put it separately. In addition, these buttons might change in a near future, and we didn't want to spend time adapting everything before having the final version.

setGurbData({
name: data?.name,
state: data?.state,
completedPercentage: data?.assigned_betas_percentage
Copy link
Contributor

Choose a reason for hiding this comment

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

If data is empty or field doesn't exists in original model it can produce an unexpected error. See our other comment in ProgressWarning component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We think that we should deal with the empty data and other errors at higher level, for instance, if there is no data a pop up will appear and show you the error (let's see what our UXer has to say on how to show the errors). Regarding the unexpected error: the idea of using data?.name_of_field is exactly that, it will return an undefined and will not through an error.

height={100}
value={completed}
text={`${completed}%`}
cornerRadius="50%"
Copy link
Contributor

Choose a reason for hiding this comment

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

If completed is undefined, text will show "undefined%"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see previous comment on SuppyPoint.jsx

JoanaFigueira and others added 9 commits November 13, 2024 16:19
* Add landing page components
* Add translations
* Add gurb theme
* Add missing dependencies
* Add missing fonts

Co-authored-by: Marta <[email protected]>
Co-authored-by: Joana Figueira <[email protected]>
- Add container to adjust pages
- Generate options inside chooser depending on option props
- Add off option in icons
- Add checkbox in chooser when selected

Co-authored-by: Marta <[email protected]>
* added a generic component called ResultRequirement
* added an specific component called SuccessRequirement
* refactor of FailureRequirement using ResultRequirement
marta197 and others added 14 commits November 13, 2024 16:19
* update fontFamily

Co-authored-by: Marta <[email protected]>
Co-authored-by: Joana Figueira <[email protected]>
Co-authored-by: Joana Figueira <[email protected]>
* text errors
* cups validations
* input field translates the given errors
* added GurbLoadingContextProvider import in App
* ask backend 2km validation
* ask google api address coordinates
* use of location input component

Co-authored-by: Joana Figueira <[email protected]>
- Adapt component to gurb's design
- Remove component from contract form
@@ -82,7 +82,7 @@ export default function SomEnergiaTheme() {
}
},
shape: {
borderRadius: '0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Shape has border radius by default? otherwise is it necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The borderRadius field was defined as a string, which is not correct, and this was giving errors. Because we are not using SomenergiaTheme we decided only to take care of the error and leave the logic as it is.

/>
)
} else {
return <></>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why there is else condition if you check max and minimum when clicks the button (next, prev)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this part is a working in progress... we were concerned only with the validation part of the form. This else will change in a near future, when we continue with the development of the next steps.

return (
<>
<Header title={`${t('GURB_SUPPLY_POINT_TITLE')} ${gurbData.name}`} />
&nbsp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe is better use css styles instead &nbsp (is it responsive?), for example using flex-css.

<>
{getStep(formikProps)}
{error ? (
<></>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is not showing the error a design thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope.. it's a work in progress thing 😄

@@ -0,0 +1,41 @@
import { useContext } from 'react'
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered to use only one component for FailureRequirements and AlertRequirements only changing the specific properties? (styles)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes... but I thought this way it makes the code more readable... and I'm still not sure what will come ahead in the form design... I'll add a TO DO, to check in the future if this makes sense or not (and if not make only one component)

@@ -0,0 +1,21 @@
import ResultRequirement from './ResultRequirement'
Copy link
Contributor

Choose a reason for hiding this comment

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

🤨 this component is not used? YET

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet.. hehehhe!! we'll use it soon!

title={t('GURB_ADDRESS_TITLE')}
text={t('GURB_ADDRESS_TITLE_HELPER')}
/>
&nbsp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered to use flex-css?

const { setError, setErrorInfo } = useContext(GurbErrorContext)

const handleLightQuestion = (value) => {
setFieldValue('has_light', value)
Copy link
Contributor

Choose a reason for hiding this comment

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

[QUESTION] Why a selection is used instead of a message inform to the user that only can continue if has light?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a design decision. The user has to choose between the 2 options

})
}
}
const options = [
Copy link
Contributor

Choose a reason for hiding this comment

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

The same question, we comment in LightQuestion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a design decision. The user has to choose between the 2 options

import * as Yup from 'yup'

export const lightValidations = Yup.object().shape({
has_light: Yup.string()
Copy link
Contributor

Choose a reason for hiding this comment

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

It make sense to validate light-on/light-off and selfconsumption-on/selfconsumption-off, when the expected requirement values are light-on and selfconsumption-off?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we are validating that the user has to choose one of the allowed options (if has light or if has no light). If the user doesn't choose one of the options, it can not go any further and the next button stays un-clickable.

@JoanaFigueira JoanaFigueira merged commit cce9c04 into master Nov 21, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants