From 1e0b7b1af6a3b773f9e435901ff76b3da378bcf0 Mon Sep 17 00:00:00 2001 From: Kevin Date: Mon, 11 Sep 2023 18:37:45 +0100 Subject: [PATCH] Feature: Optimize theme switcher performance (#4124) Because: * There is a noticeable delay between switching the theme and the theme icon updating * Related to: https://github.com/TheOdinProject/theodinproject/issues/4044 This commit: * Toggles the theme class in the same turbo stream request we use to update the theme icon. * Adds a custom turbo stream action to toggle classes on elements Before: https://github.com/TheOdinProject/theodinproject/assets/7963776/6870d2a3-27af-413a-9df8-3adca85d6c4d After: https://github.com/TheOdinProject/theodinproject/assets/7963776/28cbb153-2348-4976-b11a-8245d85143fd --- .../theme/switcher_component.html.erb | 13 +--------- app/components/theme/switcher_controller.js | 13 ---------- app/controllers/themes_controller.rb | 22 ++++++++++------ app/javascript/application.js | 1 + .../src/custom_turbo_stream_actions.js | 11 ++++++++ app/views/layouts/application.html.erb | 2 +- app/views/themes/update.turbo_stream.erb | 7 ++++-- package.json | 2 +- spec/requests/themes_spec.rb | 25 +++++++++++++++++++ 9 files changed, 60 insertions(+), 36 deletions(-) delete mode 100644 app/components/theme/switcher_controller.js create mode 100644 app/javascript/src/custom_turbo_stream_actions.js create mode 100644 spec/requests/themes_spec.rb diff --git a/app/components/theme/switcher_component.html.erb b/app/components/theme/switcher_component.html.erb index 26d9242d7c..ee8f34aedb 100644 --- a/app/components/theme/switcher_component.html.erb +++ b/app/components/theme/switcher_component.html.erb @@ -1,15 +1,4 @@ -<%= link_to( - themes_path(theme: other_theme.name), - class: yass(link: type), - data: { - 'turbo-method' => :put, - 'turbo-frame' => '_top', - controller: 'theme--switcher', - 'theme--switcher-current-theme-value' => current_theme.name, - 'theme--switcher-other-theme-value' => other_theme.name, - action: 'click->theme--switcher#toggle' - } - ) do %> +<%= link_to themes_path(theme: other_theme.name), class: yass(link: type), data: { turbo_method: :put } do %> <%= inline_svg_tag "icons/#{current_theme.icon}.svg", class: yass(icon: type), aria: true, title: 'theme icon' %> <%= text unless icon_only? %> <% end %> diff --git a/app/components/theme/switcher_controller.js b/app/components/theme/switcher_controller.js deleted file mode 100644 index 5e5a71643d..0000000000 --- a/app/components/theme/switcher_controller.js +++ /dev/null @@ -1,13 +0,0 @@ -import { Controller } from '@hotwired/stimulus'; - -export default class SwitcherController extends Controller { - static values = { - currentTheme: String, - otherTheme: String, - }; - - toggle() { - document.documentElement.classList.remove(this.currentThemeValue); - document.documentElement.classList.add(this.otherThemeValue); - } -} diff --git a/app/controllers/themes_controller.rb b/app/controllers/themes_controller.rb index b81a93dcf5..cbe303de01 100644 --- a/app/controllers/themes_controller.rb +++ b/app/controllers/themes_controller.rb @@ -1,16 +1,24 @@ class ThemesController < ApplicationController + before_action :cache_previous_theme + def update - theme = params[:theme] + @new_theme = Users::Theme.for(params[:theme]) - if Users::Theme.exists?(theme) - change_current_theme(theme) + respond_to do |format| + if @new_theme.present? + change_current_theme(params[:theme]) - respond_to do |format| - format.html { redirect_back(fallback_location: root_path) } format.turbo_stream + else + flash.now[:alert] = "Sorry, we can't find that theme." + format.turbo_stream { render turbo_stream: turbo_stream.update('flash-messages', partial: 'shared/flash') } end - else - redirect_back(fallback_location: root_path, alert: "Sorry, we can't find that theme.", status: :see_other) end end + + private + + def cache_previous_theme + @previous_theme = current_theme + end end diff --git a/app/javascript/application.js b/app/javascript/application.js index ac01ee7123..f80a041eda 100644 --- a/app/javascript/application.js +++ b/app/javascript/application.js @@ -22,5 +22,6 @@ import '@fortawesome/fontawesome-free/css/all.css'; import './src/js/axiosWithCsrf'; import 'controllers'; import '@hotwired/turbo-rails'; +import './src/custom_turbo_stream_actions'; Rails.start(); diff --git a/app/javascript/src/custom_turbo_stream_actions.js b/app/javascript/src/custom_turbo_stream_actions.js new file mode 100644 index 0000000000..9b233b9e0f --- /dev/null +++ b/app/javascript/src/custom_turbo_stream_actions.js @@ -0,0 +1,11 @@ +import { Turbo } from '@hotwired/turbo-rails'; + +Turbo.StreamActions.toggle_classes = function toggleClasses() { + const cssClasses = this.getAttribute('classes').split(' '); + const id = this.getAttribute('id'); + const element = document.getElementById(id); + + cssClasses.forEach((cssClass) => { + element.classList.toggle(cssClass); + }); +}; diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 52febb4b2d..49319c0ece 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -1,5 +1,5 @@ - + <% content_for :error_tracking do %> <%= render 'layouts/error_tracking' %> diff --git a/app/views/themes/update.turbo_stream.erb b/app/views/themes/update.turbo_stream.erb index c4029c7cd2..ea27c199f8 100644 --- a/app/views/themes/update.turbo_stream.erb +++ b/app/views/themes/update.turbo_stream.erb @@ -1,7 +1,10 @@ + +<%= turbo_stream_action_tag 'toggle_classes', id: 'root-element', classes: [@previous_theme, @new_theme] %> + <%= turbo_stream.update 'theme_switcher' do %> - <%= render Theme::SwitcherComponent.new(current_theme:, type: :icon_only) %> + <%= render Theme::SwitcherComponent.new(current_theme: @new_theme, type: :icon_only) %> <% end %> <%= turbo_stream.update 'theme_switcher_mobile' do %> - <%= render Theme::SwitcherComponent.new(current_theme:, type: :mobile) %> + <%= render Theme::SwitcherComponent.new(current_theme: @new_theme, type: :mobile) %> <% end %> diff --git a/package.json b/package.json index a1c17466e7..e5969b4f44 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "version": "1.0.0", "private": true, "scripts": { - "eslint": "eslint --max-warnings 0 './app/javascript/**/*.js' './app/components/**/*.js'", + "eslint": "eslint --max-warnings 0 './app/javascript/**/*.js'", "build:css": "npm-run-all --parallel \"build:css:* {@}\" --", "build:css:application": "postcss ./app/assets/stylesheets/application.postcss.css -o ./app/assets/builds/application.css", "build:css:active_admin": "sass ./app/assets/stylesheets/active_admin.scss ./app/assets/builds/active_admin.css --no-source-map --load-path=node_modules --style=compressed" diff --git a/spec/requests/themes_spec.rb b/spec/requests/themes_spec.rb new file mode 100644 index 0000000000..7e8a20ce6d --- /dev/null +++ b/spec/requests/themes_spec.rb @@ -0,0 +1,25 @@ +require 'rails_helper' + +RSpec.describe 'Themes' do + describe 'PUT #update' do + before do + cookies[:theme] = 'light' + end + + context 'when the theme is valid' do + it 'switches themes' do + expect do + put themes_path(format: :turbo_stream, params: { theme: 'dark' }) + end.to change { cookies[:theme] }.from('light').to('dark') + end + end + + context 'when the theme is invalid' do + it "doesn't switch themes" do + expect do + put themes_path(format: :turbo_stream, params: { theme: 'invalid' }) + end.not_to change { cookies[:theme] } + end + end + end +end