-
Notifications
You must be signed in to change notification settings - Fork 317
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 Terms of Service Dialog #9975
Conversation
} | ||
}) | ||
.then(data => { | ||
invariant(data != null, 'Invalid terms of service response') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@somebody1234 I think it's time to add a schema-validation library, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have ajv as a dependency, is that not sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess ajv doesn't do type predicates...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess ajv should be sufficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need ajv though?
app/ide-desktop/lib/dashboard/src/modals/TermsOfServiceModal.tsx
Outdated
Show resolved
Hide resolved
app/ide-desktop/lib/dashboard/src/providers/LocalStorageProvider.tsx
Outdated
Show resolved
Hide resolved
app/ide-desktop/lib/dashboard/src/providers/LocalStorageProvider.tsx
Outdated
Show resolved
Hide resolved
CR: mostly fine apart from the points above?
ideally we'd be showing the EULA in the app itself - I'm not sure whether an iframe works both in browser and in Electron, but if it is then that would be sufficient IMO (although we would probably want a special HTML page without the website header and footer, alongside the existing one |
@PabloBuchu see the PR description:
|
@somebody1234 , @PabloBuchu just merged the changes on the website and I expect this PR to be ready for QA in a few minutes. |
PR is ready for review/QA |
@MrFlashAccount truly minor things. When I hit Also I think the checkbox should toggle when I click on text not only exactly in the square |
True, we're currently limited with the components we have. Eventually, we'll improve this. I moved this out of scope because this will unreasonably increase the scope of the PR. |
We currently focus on the checkbox but it is barely visible. Let's add an error message |
not sure this is true, a plain HTML |
React aria expects the label to be passed as a child: https://react-spectrum.adobe.com/react-aria/Checkbox.html#example Reimplementing the checkbox will increase the scope of the feature and delay the release
|
efdf4ec
to
075aba7
Compare
HTML |
@somebody1234 can we approve the code and QA then? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CR ✅
QA not done (it's late here)
ready to merge. please fix failing ci checks |
ebd8559
to
b4ec86c
Compare
8335254
to
e64e699
Compare
e64e699
to
ba0620e
Compare
Pull Request Description
Tl;dr
Closes: enso-org/cloud-v2#1228
This PR adds a new DIalog that requires user to submit terms and conditions
Demo Presentation
Note during the demo the changes on the website were not merged yet, so when I clicked "view terms and conditions" link, I was redirected to the / page. Once the changes on the website are merged, this link will lead to the "Terms and Conditions" page
CleanShot.2024-05-17.at.11.55.32.mp4
This Change:
What this change does in the larger context. Specific details to highlight for review:
eula.json
from the website with hash based on EULA content) and if the hash is different from the stored one, show the TOS dialogTest Plan:
We expect to test the changes after merging https://github.com/enso-org/website/pull/7
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.