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 incognito mode #1450

Merged
merged 20 commits into from
Aug 14, 2020
Merged

Feature incognito mode #1450

merged 20 commits into from
Aug 14, 2020

Conversation

flbulgarelli
Copy link
Member

@flbulgarelli flbulgarelli commented Aug 4, 2020

📝 Details

This PR is mostly client-side, but requires the new Mumuki::Domain::Incognito user, that allows to polymorphically deal with a null-user. On the client side, this feature leverages the new mumuki.SubmissionsStore in order to render the progress - both status bar and current exercise content when exercise page is loaded.

In addition to such functional changes, there we some code improvements:

  • new JS domain tests were added
  • new JS ui tests were added
  • a all js code has been properly loaded without errors has been added
  • a linter has been added
  • the page and editor modules have been refactored
  • a new editors module has been introduced, that makes easier to polimorphically handle standard and custom editors

🔙 Backward compatibility

This PR breaks some backward compatibility in the JS side - it changes the submissions API - and therefore this PR is not backwards compatible

🔜 Future work

This PR is inspired in #1434 and thus it does not render progress on non-exercises pages. It may be generalized in the future. This will be related to the #1363 issue and all the server-side progress efforts. Likewise, this PR does neither implement the "next exercise" logic yet, nor a sync-after-login logic

⚠️ Warnings

* This module allows to read and write the current editor's contents
* regardless if it is an standard editor or a custom editor
*/
mumuki.editors = {
Copy link
Member Author

Choose a reason for hiding this comment

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

You will notice that I din't use a module-lambda here, just because it was not necessary. This is a direct consequence of building more cohesive module objects - only one module per file - using ES6 constructs, and clearly separating the initialization code - which is run only once - from the loading - mumuki.load - code - which is loaded on page load and after turbolinks load event.

I think we should tend to favor this construct when just a bunch of related functions are required, since it is less cumbersome.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, didn't see this one. Never mind my two comments related to the lambda function

@flbulgarelli flbulgarelli marked this pull request as ready for review August 10, 2020 15:20
@flbulgarelli flbulgarelli force-pushed the feature-userless-mode branch from 50a3d18 to c1e943f Compare August 10, 2020 18:00
Copy link
Contributor

@luchotc luchotc left a comment

Choose a reason for hiding this comment

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

Niceee 💯 🥇 🔝


CsrfToken.prototype = {
newRequest: function (data) {
mumuki.CsrfToken = (() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this notation still quite cumbersome 😕
Is there any reason not to do something like:

mumuki.CsrfToken = class { 
   ...
}

*
* CustomEditor sources are cleared after page reload even when using turbolinks
*/
mumuki.CustomEditor = (() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.
We are defining an anonymous function that creates and object and returns it.

Isn't more straight-forward ¿and produces the same result?
doing:

mumuki.CustomEditor = {
  ...
}

We would need to change all CustomEditor calls for this and call mumuki.load outside it.

Sorry if you already consider doing this and found out that it's not gonna work!

@@ -0,0 +1,77 @@
/** @type {boolean} */
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@@ -1,10 +1,14 @@
module LocaleHelper
def locale_tags
module GlobalsHelper
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

@@ -17,7 +17,7 @@
<% end %>
</div>
<div class="mu-navbar-avatar">
<% if current_user? %>
<% if current_user? && !current_user.incognito? %>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not necessary now but maybe we should refactor a bit this section and move it to a helper to make it incognitable 😛 as well without adding this condition

spec/javascripts/editors-spec.js Outdated Show resolved Hide resolved
@@ -0,0 +1,54 @@
describe('editors', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 👏 👏

@@ -0,0 +1,6 @@
describe("global loading", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Niceee

@flbulgarelli flbulgarelli force-pushed the feature-userless-mode branch from dc92ed7 to 3a6109d Compare August 13, 2020 19:10
@flbulgarelli flbulgarelli merged commit c2d54b0 into master Aug 14, 2020
@flbulgarelli flbulgarelli deleted the feature-userless-mode branch August 14, 2020 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants