Private tab/Session tab icon needs right padding before text #6031

jkup · 2016-12-05T22:46:29Z

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

Fix #5936

Auditors @bradleyrichter @bsclifton

Test Plan:

Open Brave
Open a private tab
Open a new session
Open a tab that plays audio
Make sure there is space after the private-eye, session-person and audio icon like in the screenshot below.

jkup · 2016-12-06T01:08:15Z

@bsclifton updated!

bsclifton · 2016-12-06T08:17:27Z

I pulled this up and tried a few configurations and got it to break

In the above pic, notice how the audio icon has no margin-left ☹️ Here's my thoughts:

we can consider making each icon that can go into a tab the same element type and give each a common classname. Right now, private/session are divs and the audio is a span
we can then use nth selectors to optionally add the left margin 😄

(if it's easy enough, this might be a great chance to try it out as an Aphrodite component; it could be a new tabIcon component or similar)

bsclifton · 2016-12-07T17:20:09Z

@jkup I saw this is using the Aphrodite components! Let me know when it's ready for review 😄 (PS- I saw the audio icon was accidentally checked in as pink)

bsclifton · 2016-12-08T00:37:59Z

js/components/tabIcon.js

+
+class AudioTabIcon extends ImmutableComponent {
+  render () {
+    return <TabIcon withBlueIcon {...this.props} />


😻 (love all the things that happen with this one line)

I wonder if there's a clean way to eliminate the mapping, since it's almost redundant;
ex: this returns withBlueIcon, which then maps itself to styles.blueIcon...

Can we have an array of styles? and then expand using a spread (in the parent)
<TabIcon customStyles={[styles.blueIcon]} {...this.props} />

===>

class TabIcon extends ImmutableComponent { //... render() { const styles = [].concat(this.props.customStyles || []) className = css(...styles) //...

bsclifton · 2016-12-08T01:12:49Z

js/components/styles/global.js

+  statsLightGray: '#999999',
+  defaultIconBackground: '#F7F7F7'
+}
+


I mentioned over Slack, but these may (or may not) be better if they were partitioned into logical groups, like color, zindex, breakpoint, etc. Pros of this:

could be split into another file easily

if (in the future) there were no references in the code to that file, standard.js would raise a lint error / webpack should exclude it

Curious what others think? @cezaraugusto @bbondy @luixxiul

For me personally, I'm good with this as-is: the list is fairly short though and I think it may make more sense to update this in iterations 😄

I updated it with my understanding of what would be good! Would love to hear comments!

bsclifton · 2016-12-08T01:17:29Z

js/components/tabIcon.js

@@ -0,0 +1,47 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


Just noticed this now- the new preferred location for components would be in:
app/renderer/components 😄

This lets us easily break up the rendering portion (client side JavaScript) and the browser portion (which has access to node / makes use of muon (electron))

bsclifton · 2016-12-08T01:31:03Z

js/components/tab.js

-          this.props.tab.get('audioMuted')
-      })}
+      if (this.props.tab.get('audioPlaybackActive') && !this.props.tab.get('audioMuted')) {
+        iconClass = 'fa fa-volume-up'


These class names (along with the other FontAwesome class names) might be a natural fit into the global styles?

For example:

const globalStyles = { //.. classAudioPlayingIcon: 'fa fa-volume-up', classAudioMutedIcon: 'fa fa-volume-off', //... }

bsclifton · 2016-12-08T01:36:29Z

js/components/tab.js

@@ -18,6 +18,8 @@ const dnd = require('../dnd')
 const windowStore = require('../stores/windowStore')


If you move the new tabIcon component to the new location (app/renderer/components), you may consider moving this too 😄 If moved, you'll have to update the paths for the requires here and then also anywhere that includes this (ex: tabs, pinnedtabs)

If it's all good with you, I might try moving tab.js later. I tried moving it now but it has a lot of deps to move around.

sounds good! 😄

bsclifton · 2016-12-08T01:40:05Z

Comments left... great job! 😄 Using Aphrodite will be a huge step forward with regard to stabilizing and encapsulating our styles (which gives us more confidence about making future changes)

bsclifton · 2016-12-08T17:59:15Z

@cezaraugusto did you want to check this out too? I know you're working in the same code right now 😄

(otherwise, LGTM!)

bradleyrichter · 2016-12-08T18:14:44Z

I made some design changes that should help clean up some of our issues too.

When should we roll it in?

bsclifton · 2016-12-09T06:09:19Z

@bradleyrichter @cezaraugusto is working on that issue; I'll share your image there 😄

jkup added design A design change, especially one which needs input from the design team. feature/tabsbar QA/test-plan-specified labels Dec 5, 2016

jkup added this to the 0.13.0 milestone Dec 5, 2016

bsclifton assigned bsclifton and bradleyrichter Dec 5, 2016

jkup force-pushed the private-session-padding branch from d6ac1c8 to 3a7cf28 Compare December 6, 2016 01:06

bsclifton force-pushed the private-session-padding branch from 3a7cf28 to b2c270b Compare December 6, 2016 08:01

bsclifton mentioned this pull request Dec 6, 2016

Refine tab's element position #5918

Closed

3 tasks

jkup force-pushed the private-session-padding branch from b2c270b to fa5e5af Compare December 7, 2016 06:13

jkup changed the title ~~Private tab/Session tab icon needs right padding before text~~ [WiP] Private tab/Session tab icon needs right padding before text Dec 7, 2016

jkup force-pushed the private-session-padding branch 3 times, most recently from 1762ce1 to 8c42092 Compare December 8, 2016 00:26

bsclifton reviewed Dec 8, 2016

View reviewed changes

Private tab/Session tab icon needs right padding before text

d8f5292

jkup force-pushed the private-session-padding branch from 8c42092 to d8f5292 Compare December 8, 2016 04:24

bsclifton changed the title ~~[WiP] Private tab/Session tab icon needs right padding before text~~ Private tab/Session tab icon needs right padding before text Dec 9, 2016

bsclifton merged commit 4651b19 into master Dec 9, 2016

bsclifton deleted the private-session-padding branch December 9, 2016 06:09

This was referenced Dec 21, 2016

Manual tests for Windows ia-32 0.13.0 Preview 6 #6335

Closed

Manual tests for Linux Mint 0.13.0 Preview 6 #6337

Closed

Manual tests for Windows x64 0.13.0 Preview 6 #6334

Closed

Manual tests for OS X 0.13.0 Preview 6 #6332

Closed

luixxiul mentioned this pull request Jan 16, 2017

Private tab/Session tab icon needs right padding before text #5936

Closed

luixxiul removed the QA/test-plan-specified label Jan 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Private tab/Session tab icon needs right padding before text #6031

Private tab/Session tab icon needs right padding before text #6031

jkup commented Dec 5, 2016 •

edited

Loading

jkup commented Dec 6, 2016

bsclifton commented Dec 6, 2016 •

edited

Loading

bsclifton commented Dec 7, 2016

bsclifton Dec 8, 2016 •

edited

Loading

bsclifton Dec 8, 2016 •

edited

Loading

bsclifton Dec 8, 2016

jkup Dec 8, 2016

bsclifton Dec 8, 2016

jkup Dec 8, 2016

bsclifton Dec 8, 2016 •

edited

Loading

bsclifton Dec 8, 2016

jkup Dec 8, 2016

bsclifton Dec 8, 2016

bsclifton commented Dec 8, 2016

bsclifton commented Dec 8, 2016 •

edited

Loading

bradleyrichter commented Dec 8, 2016

bsclifton commented Dec 9, 2016

		@@ -0,0 +1,47 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -18,6 +18,8 @@ const dnd = require('../dnd')
		const windowStore = require('../stores/windowStore')

Private tab/Session tab icon needs right padding before text #6031

Private tab/Session tab icon needs right padding before text #6031

Conversation

jkup commented Dec 5, 2016 • edited Loading

jkup commented Dec 6, 2016

bsclifton commented Dec 6, 2016 • edited Loading

bsclifton commented Dec 7, 2016

bsclifton Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

bsclifton Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

bsclifton Dec 8, 2016

Choose a reason for hiding this comment

jkup Dec 8, 2016

Choose a reason for hiding this comment

bsclifton Dec 8, 2016

Choose a reason for hiding this comment

jkup Dec 8, 2016

Choose a reason for hiding this comment

bsclifton Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

bsclifton Dec 8, 2016

Choose a reason for hiding this comment

jkup Dec 8, 2016

Choose a reason for hiding this comment

bsclifton Dec 8, 2016

Choose a reason for hiding this comment

bsclifton commented Dec 8, 2016

bsclifton commented Dec 8, 2016 • edited Loading

bradleyrichter commented Dec 8, 2016

bsclifton commented Dec 9, 2016

jkup commented Dec 5, 2016 •

edited

Loading

bsclifton commented Dec 6, 2016 •

edited

Loading

bsclifton Dec 8, 2016 •

edited

Loading

bsclifton Dec 8, 2016 •

edited

Loading

bsclifton Dec 8, 2016 •

edited

Loading

bsclifton commented Dec 8, 2016 •

edited

Loading