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

Document type conversion UX/UI #1181

Merged

Conversation

hexaltation
Copy link
Collaborator

Context

Document Type conversion is available through API calls and API console. As a non technical document owner I want to be able to convert my document to Template or Tutorial and vice-versa easily in the UI.

Proposed solution

This PR implements figma design by @lusebille https://www.figma.com/design/wcpetFt6aOKzTszcvPPWLQ/%5B05%2F24%5D-Grist-Design?node-id=559-80548&t=1gSxqwNd6ZsiXdFO-1

The only point not implemented is the notification after modal confirmation.
As the page needs to be reloaded its not simple to notify for the successful change.

Related issues

fixes #1015

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this is not relevant here
  • 🙋 no, because I need help

Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

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

Some remarks so far

app/client/models/DocPageModel.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
@hexaltation hexaltation force-pushed the issue-1015-convert-document-to-template-tuto branch from 71f3678 to 5d3449a Compare September 4, 2024 12:20
Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

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

Good job! This feature is promising!

Here are some comments I got.

Something that surprised me a bit is that when I:

  • converted a tutorial from the Template org to a regular doc;
  • made some changes;
  • converted back to a tutorial;

The tutorial did not run. Any idea?

I used the Grist Basic doc for that.

test/nbrowser/DocTypeConversion.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
@hexaltation hexaltation force-pushed the issue-1015-convert-document-to-template-tuto branch from 5d3449a to 9a21be7 Compare September 12, 2024 11:58
@fflorent fflorent self-requested a review September 20, 2024 14:09
Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

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

I think there is an issue with the current implementation, as I cannot convert documents to tutorials or template when I tested your work manually. I suggested a change in the test to reveal the issue.

test/nbrowser/DocTypeConversion.ts Outdated Show resolved Hide resolved
test/nbrowser/DocTypeConversion.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
@fflorent fflorent self-requested a review October 8, 2024 14:26
@hexaltation hexaltation force-pushed the issue-1015-convert-document-to-template-tuto branch from 92c4b15 to 6750cfd Compare October 9, 2024 09:38
@hexaltation
Copy link
Collaborator Author

The two broken tests seems not to be linked with code modified by this PR.
Do you agree with that @fflorent ?

Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

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

If you need help or me to take some remarks, don't hesitate.

app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
test/nbrowser/DocTypeConversion.ts Outdated Show resolved Hide resolved
test/nbrowser/DocTypeConversion.ts Outdated Show resolved Hide resolved
test/nbrowser/DocTypeConversion.ts Outdated Show resolved Hide resolved
test/nbrowser/DocTypeConversion.ts Outdated Show resolved Hide resolved
test/nbrowser/DocTypeConversion.ts Outdated Show resolved Hide resolved
test/nbrowser/DocTypeConversion.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

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

Great job! 👏

app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
test/nbrowser/DocTypeConversion.ts Outdated Show resolved Hide resolved
test/nbrowser/DocTypeConversion.ts Show resolved Hide resolved
test/nbrowser/DocTypeConversion.ts Show resolved Hide resolved
@hexaltation hexaltation force-pushed the issue-1015-convert-document-to-template-tuto branch from 769213b to 2d14a48 Compare October 23, 2024 12:37
@berhalak berhalak self-assigned this Oct 30, 2024
@hexaltation hexaltation added the preview Launch preview deployment of this PR label Oct 30, 2024
Copy link
Contributor

Deployed commit 2d14a48d6ed12176398a0048f86de3173b9bb432 as https://grist-hexaltation-grist-core-issue-1015-convert-document-to-template-tuto.fly.dev (until 2024-11-29T16:55:58.444Z)

app/client/models/DocPageModel.ts Outdated Show resolved Hide resolved
app/client/ui/AdminPanelCss.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
app/client/ui2018/checkbox.ts Outdated Show resolved Hide resolved
app/client/ui2018/checkbox.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
app/client/ui/DocumentSettings.ts Outdated Show resolved Hide resolved
@hexaltation hexaltation force-pushed the issue-1015-convert-document-to-template-tuto branch from 2d14a48 to 0bc761c Compare October 31, 2024 12:11
Copy link
Contributor

Deployed commit 0bc761c56ef32fd350a4a42044a6c2824711ecc1 as https://grist-hexaltation-grist-core-issue-1015-convert-document-to-template-tuto.fly.dev (until 2024-11-30T12:16:27.910Z)

static/icons/icons.css Outdated Show resolved Hide resolved
Copy link
Contributor

Deployed commit 4eac14dba424a46b046c0acfad06dde6afcd4114 as https://grist-hexaltation-grist-core-issue-1015-convert-document-to-template-tuto.fly.dev (until 2024-11-30T13:00:55.012Z)

Copy link
Contributor

Deployed commit 801611e40801277a37c3ea369f3d561b80a17601 as https://grist-hexaltation-grist-core-issue-1015-convert-document-to-template-tuto.fly.dev (until 2024-11-30T19:47:19.979Z)

fix: url deduction for page reload to apply new document type
TODO onboarding popup

TODO better waiting for url change
remove uneeded async and better function naming
fix: various nitpicks
@hexaltation hexaltation force-pushed the issue-1015-convert-document-to-template-tuto branch from d0c5dcd to 860d8c1 Compare November 26, 2024 14:39
Copy link
Contributor

Deployed commit 860d8c1d389c4cdd722d18ea4ffc8079c8088d01 as https://grist-hexaltation-grist-core-issue-1015-convert-document-to-template-tuto.fly.dev (until 2024-12-26T14:44:17.517Z)

@berhalak berhalak merged commit 123ad18 into gristlabs:main Nov 27, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Launch preview deployment of this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Converting documents to tutorials or templates through the UI
5 participants