-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
Remove demo code #3538
Remove demo code #3538
Conversation
I assume we want to wait until the April release branch is cut (cc @ghislaineguerin) before we merge this. |
Definitely. @pavish Any opinions on whether we should remove the demo-mode-related translations? I know we paid for them, but we can always recover them through git or other means when/if we want them back. |
Yes, we should remove any key we're no longer using. We can always recover them if needed, Transifex also has a memory feature for previously translated strings, apart from git. |
Agreed that paying for the translations isn't a reason to keep them around if we no longer need them. |
Okay, this is now ready for review. From @pavish , I'm just requesting a quick check to make sure there aren't any obvious problems where the front end might break because it expects something (e.g., a flag or variable) to be available from the back end. |
@kgodey @mathemancer I have cut the release 2 days ago. |
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.
@mathemancer The code changes look good to me. I have a couple comments.
{% if live_demo_mode %} | ||
<div class="unsupported-device"> | ||
<div class="warning-icon">⚠️</div> | ||
<p class="title">{% translate "Unsupported Screen Size" %}</p> | ||
<p> | ||
{% translate "Mathesar is a spreadsheet-like application with a rich UI that does not yet function well on screens this small. Improved support for mobile devices is on our" %} | ||
|
||
<a href="https://mathesar.org/roadmap.html" target="_blank"> | ||
{% translate "roadmap" %} | ||
</a>. | ||
</p> | ||
<p> | ||
{% translate "You can still use this demo site, but some features may not work correctly. We encourage you to try Mathesar on a device with a larger screen." %} | ||
</p> | ||
</div> | ||
<div class="tutorial align-center"> | ||
<div class="header"> | ||
<strong>{% translate "Live Demo Mode" %}</strong> | ||
</div> | ||
<div class="body"> | ||
{% translate "Mathesar's live demo is available to anyone to try out." %} | ||
|
||
{% if live_demo_username and live_demo_password %} | ||
{% translate "Use the following credentials to login" %}: | ||
<ul> | ||
<li>{% translate "Username" %}: <strong>{{live_demo_username}}</strong></li> | ||
<li>{% translate "Password" %}: <strong>{{live_demo_password}}</strong></li> | ||
</ul> | ||
{% endif %} | ||
|
||
{% translate "Keep in mind that the data in the live demo is reset regularly." %} | ||
</div> | ||
</div> | ||
{% endif %} | ||
|
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 would have to figure out a different way to mention that the demo data is reset regularly to users accessing the demo site. Do we have an approach figured out on how to do that?
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 don't have an approach figured out yet. I figure let's get everything out for now, and then only add things back into the service if absolutely needed later. Couldn't we have a custom front page served by the demo microservice? It could pull the needed assets from the "real service". Not sure, though.
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 think having a custom front page should be fine.
However, we currently show a banner on the app while someone is viewing the demo. This PR removes that banner as well. I'm cool with removing it and adding it later based on requirements. I do think this needs to be tracked in a separate issue.
@pavish I made the requested change. |
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.
Looks good to me!
Waiting on your re-review @pavish, please merge this if you don't have any more concerns. |
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.
@mathemancer Thanks, looks great!
We will have to create an issue to track this: #3538 (comment)
Fixes #3531
This PR removes all Demo code which is otherwise unused from Mathesar.
Technical details
Some demo code was used for example datasets in other context, and so was moved to a different location rather than removed entirely.
Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin