Updating naming conventions for UI and region selectors

It's a lot better to use a data-region or data-ui attribute because it keeps classes reserved for CSS. Reduces confusion in the long run, but I'd like to make the whole process a bit more obvious.
MeoMix · May 16, 2015 · 02d8f8b · 02d8f8b
1 parent cbaaa6e
commit 02d8f8b
Show file tree

Hide file tree

Showing 92 changed files with 495 additions and 569 deletions.
diff --git a/Streamus Chrome Extension.csproj b/Streamus Chrome Extension.csproj
@@ -80,7 +80,7 @@
     <Content Include="src\js\foreground\model\contextMenuAction.js" />
     <Content Include="src\js\foreground\model\mediaSourceWrapper.js" />
     <Content Include="src\js\foreground\model\sourceBufferWrapper.js" />
-    <Content Include="src\js\foreground\enum\keyboardKey.js" />
+    <Content Include="src\js\foreground\enum\keyCode.js" />
     <Content Include="src\js\foreground\model\tooltip.js" />
     <Content Include="src\js\foreground\view\dialog\aboutStreamusDialogView.js" />
     <Content Include="src\js\foreground\view\dialog\aboutStreamusView.js" />

diff --git a/src/js/background/view/backgroundAreaView.js b/src/js/background/view/backgroundAreaView.js
@@ -13,12 +13,13 @@
         regions: function(options) {
             return {
                 youTubePlayerRegion: {
-                    el: '#' + this.id + '-youTubePlayerRegion',
+                    el: '[data-region=youTubePlayer]',
                     regionClass: YouTubePlayerRegion,
+                    //  TODO: feels weird to need to do this... all other regions aren't functions
                     youTubePlayer: options.model.get('youTubePlayer')
                 },
                 clipboardRegion: {
-                    el: '#' + this.id + '-clipboardRegion',
+                    el: '[data-region=clipboard]',
                     regionClass: ClipboardRegion
                 }
             };

diff --git a/src/js/foreground/enum/keyCode.js b/src/js/foreground/enum/keyCode.js
@@ -0,0 +1,7 @@
+define({
+    Space: 32,
+    ArrowLeft: 37,
+    ArrowUp: 38,
+    ArrowRight: 39,
+    ArrowDown: 40
+});
diff --git a/src/js/foreground/enum/keyboardKey.js b/src/js/foreground/enum/keyboardKey.js
diff --git a/src/js/foreground/view/appBar/adminMenuAreaView.js b/src/js/foreground/view/appBar/adminMenuAreaView.js
@@ -22,14 +22,14 @@
         },
 
         ui: {
-            menuButton: '.menuButton',
-            menu: '.menu',
-            settings: '.settings',
-            browserSettings: '.browserSettings',
-            openInTab: '.openInTab',
-            keyboardShortcuts: '.keyboardShortcuts',
-            aboutStreamus: '.aboutStreamus',
-            restart: '.reload'
+            menuButton: '[data-ui~=menuButton]',
+            menu: '[data-ui~=menu]',
+            settings: '[data-ui~=settings]',
+            browserSettings: '[data-ui~=browserSettings]',
+            openInTab: '[data-ui~=openInTab]',
+            keyboardShortcuts: '[data-ui~=keyboardShortcuts]',
+            aboutStreamus: '[data-ui~=aboutStreamus]',
+            restart: '[data-ui~=reload]'
         },
 
         events: {

diff --git a/src/js/foreground/view/appBar/appBarView.js b/src/js/foreground/view/appBar/appBarView.js
@@ -29,29 +29,25 @@
             };
         },
 
-        regions: function() {
-            return {
-                playlistTitleRegion: '#' + this.id + '-playlistTitleRegion',
-                volumeAreaRegion: '#' + this.id + '-volumeAreaRegion',
-                adminMenuAreaRegion: '#' + this.id + '-adminMenuAreaRegion',
-                previousButtonRegion: '#' + this.id + '-previousButtonRegion',
-                playPauseButtonRegion: '#' + this.id + '-playPauseButtonRegion',
-                nextButtonRegion: '#' + this.id + '-nextButtonRegion'
-            };
-        },
-
-        ui: function() {
-            return {
-                searchInput: '#' + this.id + '-searchInput',
-                showSearchButton: '#' + this.id + '-showSearchButton',
-                hideSearchButton: '#' + this.id + '-hideSearchButton',
-                showPlaylistsAreaButton: '#' + this.id + '-showPlaylistsAreaButton',
-                hidePlaylistsAreaButton: '#' + this.id + '-hidePlaylistsAreaButton',
-                //  TODO: I don't like regions being manipulated in UI they should be stateless.
-                //  To achieve this, I'll need to make playlistTitleRegion and searchInputRegion the same region and swap views out.
-                playlistTitleRegion: '#' + this.id + '-playlistTitleRegion',
-                searchInputRegion: '#' + this.id + '-searchInputRegion'
-            };
+        regions: {
+            playlistTitleRegion: '[data-region=playlistTitle]',
+            volumeAreaRegion: '[data-region=volumeArea]',
+            adminMenuAreaRegion: '[data-region=adminMenuArea]',
+            previousButtonRegion: '[data-region=previousButton]',
+            playPauseButtonRegion: '[data-region=playPauseButton]',
+            nextButtonRegion: '[data-region=nextButton]'
+        },
+
+        ui: {
+            searchInput: '[data-ui~=searchInput]',
+            showSearchButton: '[data-ui~=showSearchButton]',
+            hideSearchButton: '[data-ui~=hideSearchButton]',
+            showPlaylistsAreaButton: '[data-ui~=showPlaylistsAreaButton]',
+            hidePlaylistsAreaButton: '[data-ui~=hidePlaylistsAreaButton]',
+            //  TODO: I don't like regions being manipulated in UI they should be stateless.
+            //  To achieve this, I'll need to make playlistTitleRegion and searchInputRegion the same region and swap views out.
+            playlistTitleRegion: '[data-ui~=playlistTitleRegion]',
+            searchInputRegion: '[data-ui~=searchInputRegion]'
         },
 
         events: {

diff --git a/src/js/foreground/view/appBar/playPauseButtonView.js b/src/js/foreground/view/appBar/playPauseButtonView.js
@@ -15,11 +15,9 @@
             playIcon: _.template(PlayIconTemplate)()
         },
 
-        ui: function() {
-            return {
-                playIcon: '#' + this.id + '-playIcon',
-                pauseIcon: '#' + this.id + '-pauseIcon'
-            };
+        ui: {
+            playIcon: '[data-ui~=playIcon]',
+            pauseIcon: '[data-ui~=pauseIcon]'
         },
 
         events: {

diff --git a/src/js/foreground/view/appBar/playlistTitleView.js b/src/js/foreground/view/appBar/playlistTitleView.js
@@ -5,9 +5,13 @@
     var PlaylistTitleTemplate = require('text!template/appBar/playlistTitle.html');
 
     var PlaylistTitleView = Marionette.ItemView.extend({
-        className: 'contentBar-title text u-textOverflowEllipsis js-textTooltipable',
+        className: 'contentBar-title text u-textOverflowEllipsis',
         template: _.template(PlaylistTitleTemplate),
 
+        attributes: {
+            'data-ui': 'textTooltipable'
+        },
+
         modelEvents: {
             'change:title': '_onChangeTitle'
         },

diff --git a/src/js/foreground/view/appBar/volumeAreaView.js b/src/js/foreground/view/appBar/volumeAreaView.js
@@ -22,17 +22,15 @@ define(function(require) {
             };
         },
 
-        ui: function() {
-            return {
-                volumeProgress: '#' + this.id + '-volumeProgress',
-                volumeRange: '#' + this.id + '-volumeRange',
-                volumeButton: '#' + this.id + '-volumeButton',
-                slidePanel: '#' + this.id + '-slidePanel',
-                volumeIconUp: '#' + this.id + '-volumeIcon--up',
-                volumeIconDown: '#' + this.id + '-volumeIcon--down',
-                volumeIconOff: '#' + this.id + '-volumeIcon--off',
-                volumeIconMute: '#' + this.id + '-volumeIcon--mute'
-            };
+        ui: {
+            volumeProgress: '[data-ui~=volumeProgress]',
+            volumeRange: '[data-ui~=volumeRange]',
+            volumeButton: '[data-ui~=volumeButton]',
+            slidePanel: '[data-ui~=slidePanel]',
+            volumeIconUp: '[data-ui~=volumeIcon--up]',
+            volumeIconDown: '[data-ui~=volumeIcon--down]',
+            volumeIconOff: '[data-ui~=volumeIcon--off]',
+            volumeIconMute: '[data-ui~=volumeIcon--mute]'
         },
 
         events: {

diff --git a/src/js/foreground/view/behavior/itemViewMultiSelect.js b/src/js/foreground/view/behavior/itemViewMultiSelect.js
@@ -6,7 +6,7 @@
 
     var ItemViewMultiSelect = Marionette.Behavior.extend({
         ui: {
-            leftContent: '.listItem-leftContent'
+            leftContent: '[data-ui~=leftContent]'
         },
 
         events: {
@@ -31,7 +31,6 @@
         },
 
         onRender: function() {
-            this.$el.addClass('js-listItem--multiSelect');
             this._setSelectedClass(this.view.model.get('selected'));
         },
 

diff --git a/src/js/foreground/view/behavior/tooltipable.js b/src/js/foreground/view/behavior/tooltipable.js
@@ -3,10 +3,10 @@
 
     var Tooltipable = Marionette.Behavior.extend({
         ui: {
-            //  Children which need tooltips are decorated with the js-tooltipable class.
-            tooltipable: '.js-tooltipable',
-            //  Children which need tooltips, but also need to take into account overflowing, are decorated with the js-textTooltipable class.
-            textTooltipable: '.js-textTooltipable'
+            //  Children which need tooltips and do not need to take into account text overflowing.
+            tooltipable: '[data-ui~=tooltipable]',
+            //  Children which need tooltips, but also need to take into account overflowing.
+            textTooltipable: '[data-ui~=textTooltipable]'
         },
 
         //  Whether view's element or a descendant is showing a tooltip
@@ -29,7 +29,7 @@
         },
 
         _onMouseEnter: function(event) {
-            this._setHovered(event.target);
+            this._ensureHovered(event.target);
         },
 
         _onMouseLeave: function(event) {
@@ -39,7 +39,7 @@
 
         //  Begin the process of showing a tooltip on a given target by ensuring the target wants a tooltip
         //  Some parent views can implement the Tooltipable behavior for children without themselves needing a tooltip
-        _setHovered: function(target) {
+        _ensureHovered: function(target) {
             var $target = $(target);
 
             if (!$target.data('is-hovered')) {
@@ -99,7 +99,8 @@
 
             if (showTooltip) {
                 //  Some elements only want to show a tooltip if their text can't all be seen
-                var checkOverflow = target.classList.contains('js-textTooltipable');
+                var uiDataAttribute = target.dataset.ui;
+                var checkOverflow = !_.isUndefined(uiDataAttribute) && uiDataAttribute.indexOf('textTooltipable') !== -1;
 
                 if (checkOverflow) {
                     //  If offsetWidth is less than scrollWidth then text is being clipped.

diff --git a/src/js/foreground/view/contextMenu/contextMenuItemView.js b/src/js/foreground/view/contextMenu/contextMenuItemView.js
@@ -7,7 +7,7 @@
     var ContextMenuItemView = Marionette.ItemView.extend({
         tagName: 'li',
         className: function() {
-            var className = 'listItem listItem--small listItem--clickable js-tooltipable';
+            var className = 'listItem listItem--small listItem--clickable';
             className += this.model.get('disabled') ? ' is-disabled' : '';
             return className;
         },
@@ -19,6 +19,7 @@
 
         attributes: function() {
             return {
+                'data-ui': 'tooltipable',
                 'data-tooltip-text': this.model.get('title')
             };
         },

diff --git a/src/js/foreground/view/contextMenu/contextMenuView.js b/src/js/foreground/view/contextMenu/contextMenuView.js
@@ -11,19 +11,12 @@
         childView: ContextMenuItemView,
         childViewContainer: '@ui.contextMenuItems',
         template: _.template(ContextMenuTemplate),
-        templateHelpers: function() {
-            return {
-                viewId: this.id
-            };
-        },
         //  Used to determine whether contextMenu display should flip as to not overflow container
         containerHeight: 0,
         containerWidth: 0,
 
-        ui: function() {
-            return {
-                contextMenuItems: '#' + this.id + '-contextMenuItems'
-            };
+        ui: {
+            contextMenuItems: '[data-ui~=contextMenuItems]'
         },
 
         initialize: function(options) {

diff --git a/src/js/foreground/view/dialog/aboutStreamusView.js b/src/js/foreground/view/dialog/aboutStreamusView.js
@@ -17,8 +17,8 @@
         },
 
         ui: {
-            openHomepage: '.openHomepage',
-            openPatchNotes: '.openPatchNotes'
+            openHomepage: '[data-ui~=openHomepage]',
+            openPatchNotes: '[data-ui~=openPatchNotes]'
         },
 
         events: {

diff --git a/src/js/foreground/view/dialog/browserSettingsView.js b/src/js/foreground/view/dialog/browserSettingsView.js
@@ -15,14 +15,12 @@
             websiteEnhancementsMessage: chrome.i18n.getMessage('websiteEnhancements')
         },
 
-        regions: function() {
-            return {
-                showTextSelectionContextMenuRegion: '#' + this.id + '-showTextSelectionContextMenuRegion',
-                showYouTubeLinkContextMenuRegion: '#' + this.id + '-showYouTubeLinkContextMenuRegion',
-                showYouTubePageContextMenuRegion: '#' + this.id + '-showYouTubePageContextMenuRegion',
-                enhanceYouTubeRegion: '#' + this.id + '-enhanceYouTubeRegion',
-                enhanceBeatportRegion: '#' + this.id + '-enhanceBeatportRegion',
-            };
+        regions: {
+            showTextSelectionContextMenuRegion: '[data-region=showTextSelectionContextMenu]',
+            showYouTubeLinkContextMenuRegion: '[data-region=showYouTubeLinkContextMenu]',
+            showYouTubePageContextMenuRegion: '[data-region=showYouTubePageContextMenu]',
+            enhanceYouTubeRegion: '[data-region=enhanceYouTube]',
+            enhanceBeatportRegion: '[data-region=enhanceBeatport]'
         },
 
         initialize: function() {

diff --git a/src/js/foreground/view/dialog/createPlaylistView.js b/src/js/foreground/view/dialog/createPlaylistView.js
@@ -21,13 +21,11 @@
             };
         },
 
-        ui: function() {
-            return {
-                title: '#' + this.id + '-title',
-                titleCharacterCount: '#' + this.id + '-title-characterCount',
-                dataSource: '#' + this.id + '-dataSource',
-                dataSourceHint: '#' + this.id + '-dataSource-hint'
-            };
+        ui: {
+            title: '[data-ui~=title]',
+            titleCharacterCount: '[data-ui~=title-characterCount]',
+            dataSource: '[data-ui~=dataSource]',
+            dataSourceHint: '[data-ui~=dataSource-hint]'
         },
 
         events: {

diff --git a/src/js/foreground/view/dialog/dialogView.js b/src/js/foreground/view/dialog/dialogView.js
@@ -8,30 +8,20 @@
     var DialogView = Marionette.LayoutView.extend({
         className: 'dialog overlay overlay--faded u-transitionable transition--veryFast',
         template: _.template(DialogTemplate),
-        templateHelpers: function() {
-            return {
-                viewId: this.id
-            };
-        },
-
         contentView: null,
 
-        regions: function() {
-            return {
-                reminderRegion: '#' + this.id + '-reminderRegion',
-                contentRegion: '#' + this.id + '-contentRegion'
-            };
+        regions: {
+            reminderRegion: '[data-region=reminder]',
+            contentRegion: '[data-region=content]'
         },
 
-        ui: function() {
-            return {
-                panel: '#' + this.id + '-panel',
-                submitButton: '#' + this.id + '-submitButton',
-                cancelButton: '#' + this.id + '-cancelButton',
-                reminderCheckbox: '#' + this.id + '-reminderCheckbox',
-                closeButton: '#' + this.id + '-closeButton',
-                submittable: '.js-submittable'
-            };
+        ui: {
+            panel: '[data-ui~=panel]',
+            submitButton: '[data-ui~=submitButton]',
+            cancelButton: '[data-ui~=cancelButton]',
+            reminderCheckbox: '[data-ui~=reminderCheckbox]',
+            closeButton: '[data-ui~=closeButton]',
+            submittable: '[data-ui~=submittable]'
         },
 
         events: {
@@ -148,7 +138,6 @@
             });
 
             this.showChildView('reminderRegion', new CheckboxView({
-                id: this.id + '-reminderCheckbox',
                 model: this.reminderCheckbox
             }));
         },
@@ -174,7 +163,7 @@
         _isValid: function() {
             //  TODO: Derive this from dialog's ViewModel state instead of asking the DOM.
             //  Don't use UI here because is-invalid is appended dynamically and so I can't rely on the cache.
-            return this.$el.find('.js-submittable.is-invalid').length === 0;
+            return this.$el.find('[data-ui~=submittable].is-invalid').length === 0;
         },
 
         _hide: function() {

diff --git a/src/js/foreground/view/dialog/editPlaylistView.js b/src/js/foreground/view/dialog/editPlaylistView.js
@@ -17,11 +17,9 @@
             };
         },
 
-        ui: function() {
-            return {
-                title: '#' + this.id + '-title',
-                titleCharacterCount: '#' + this.id + '-title-characterCount'
-            };
+        ui: {
+            title: '[data-ui~=title]',
+            titleCharacterCount: '[data-ui~=title-characterCount]'
         },
 
         events: {