skin/updates.css - Issue 29592569: Issue 5943 - Implement Updates Page for Adblock Plus extension

Side by Side Diff: skin/updates.css

Issue 29592569: Issue 5943 - Implement Updates Page for Adblock Plus extension (Closed) Base URL: https://hg.adblockplus.org/adblockplusui/

Patch Set: Addressed second round of feedback Created Nov. 13, 2017, 8:47 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 @font-face
	Thomas Greiner 2017/11/20 17:06:05 I'm glad that you got rid of those because it's pr I'm glad that you got rid of those because it's pretty much impossible for me to spot them during a review.
	2 {

	3 font-family: "Source Sans Pro";

	4 font-style: normal;

	5 font-weight: 400;

	6 font-stretch: normal;

	7 src: local ("Ø"),

	8 url(fonts/SourceSansPro-Regular.woff) format("woff");

	9 }

	10

	11 @font-face

	12 {

	13 font-family: "Source Sans Pro";

	14 font-style: bold;

	15 font-weight: 600;

	16 font-stretch: normal;

	17 src: local ("Ø"),

	18 url(fonts/SourceSansPro-bold.woff) format("woff");

	19 }

	20

	21 html

	22 {

	23 font-family: "Source Sans Pro", Helvetica, Arial, sans-serif;

	24 }

	25

	26 body

	27 {

	28 margin: 0;

	29 padding: 0;

	30 }

	31

	32 #container

	33 {

	34 display: flex;

	35 flex-direction: row;

	36 align-items: stretch;

	37 }

	38

	39 .column

	40 {

	41 height: 100vh;

	42 display: flex;

	43 align-items: center;

	44 justify-content: center;

	45 flex: 1;

	46 }

	47

	48 #graphic-column

	49 {

	50 background-color: #8DC446;

	51
	Thomas Greiner 2017/11/14 12:25:43 Coding style: "No whitespace at the end of a line. Coding style: "No whitespace at the end of a line." See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... martin 2017/11/20 11:47:30 Done. Show quoted text On 2017/11/14 12:25:43, Thomas Greiner wrote: > Coding style: "No whitespace at the end of a line." > See > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... Done.
	52 }

	53

	54 .graphic-column

	55 {

	56 background-color: #8DC446;

	57 }

	58

	59 #content

	60 {

	61 padding: 0 3em;

	62 max-width: 760px;

	63 }

	64

	65 #content a {

	66 color: #C70D2C;

	67 text-decoration: none;

	68 }

	69

	70 #content a:hover {

	71 text-decoration: underline;

	72 }

	73

	74 #content header

	75 {

	76 margin: 0 0 2em 5em;

	77 }

	78

	79 #content header[dir="rtl"]
	Thomas Greiner 2017/11/14 12:25:41 This won't work because the "dir" attribute is on This won't work because the "dir" attribute is on the `<body>` element. Same applies to all other occurrences where you used it like this. Note that you can easily test whether it works by changing the "dir" attribute because the browser will then change the layout of the page accordingly and you'll see which things might look odd. martin 2017/11/20 11:47:31 Done. Show quoted text On 2017/11/14 12:25:41, Thomas Greiner wrote: > This won't work because the "dir" attribute is on the `<body>` element. > > Same applies to all other occurrences where you used it like this. > > Note that you can easily test whether it works by changing the "dir" attribute > because the browser will then change the layout of the page accordingly and > you'll see which things might look odd. Done.
	80 {

	81 margin: 0 5em 2em 0;
	Thomas Greiner 2017/11/14 12:25:42 Coding style: "No whitespace at the end of a line. Coding style: "No whitespace at the end of a line." See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... martin 2017/11/20 11:47:31 Done. Show quoted text On 2017/11/14 12:25:42, Thomas Greiner wrote: > Coding style: "No whitespace at the end of a line." > See > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style... Done.
	82 }

	83

	84 #content header p {

	85 margin: 0;

	86 opacity: 0.5;

	87 font-weight: 400;

	88 font-size: 1.2em;

	89 }

	90

	91 #content header h1

	92 {

	93 margin: 0;

	94 }

	95

	96 .custom-feature-entry

	97 {

	98 margin-top: 2em;

	99 }

	100

	101 .custom-feature-entry h2 {

	102 margin: 0 0 0 3.4em;
	Thomas Greiner 2017/11/14 12:25:42 Detail: This may produce unexpected results for ri Detail: This may produce unexpected results for right-to-left languages. martin 2017/11/20 11:47:33 Done. Show quoted text On 2017/11/14 12:25:42, Thomas Greiner wrote: > Detail: This may produce unexpected results for right-to-left languages. Done.
	103 }

	104

	105 .update-graphic-container

	106 {

	107 width: 560px;

	108 height: 460px;

	109 background-image: url(./updates/base-graphic.svg);
	Thomas Greiner 2017/11/14 12:25:41 Coding style: "CSS rule declaration order should f Coding style: "CSS rule declaration order should follow the WordPress CSS Coding Standards." See https://adblockplus.org/en/coding-style#html-css and https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/... martin 2017/11/20 11:47:30 Soo... Is this about the declaration order? Should Show quoted text On 2017/11/14 12:25:41, Thomas Greiner wrote: > Coding style: "CSS rule declaration order should follow the WordPress CSS Coding > Standards." > See https://adblockplus.org/en/coding-style#html-css and > https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/... Soo... Is this about the declaration order? Should I alphabetise? Thomas Greiner 2017/11/20 17:06:04 Yes, it's about the order of declaration. No, don' Show quoted text On 2017/11/20 11:47:30, martin wrote: > On 2017/11/14 12:25:41, Thomas Greiner wrote: > > Coding style: "CSS rule declaration order should follow the WordPress CSS > Coding > > Standards." > > See https://adblockplus.org/en/coding-style#html-css and > > > https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/... > > Soo... Is this about the declaration order? Should I alphabetise? Yes, it's about the order of declaration. No, don't sort them alphabetically but grouped by what's defined in the linked Wordpress coding style. So in this case the ordering should be as follows: 1. Display: display 2. Positioning: align-items, position (in any order) 3. Box model: height, width (in any order) 4. Colors and typography: background 5. Other: N/A martin 2017/11/26 17:08:37 Done. Show quoted text On 2017/11/20 17:06:04, Thomas Greiner wrote: > On 2017/11/20 11:47:30, martin wrote: > > On 2017/11/14 12:25:41, Thomas Greiner wrote: > > > Coding style: "CSS rule declaration order should follow the WordPress CSS > > Coding > > > Standards." > > > See https://adblockplus.org/en/coding-style#html-css and > > > > > > https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/... > > > > Soo... Is this about the declaration order? Should I alphabetise? > > Yes, it's about the order of declaration. > No, don't sort them alphabetically but grouped by what's defined in the linked > Wordpress coding style. > > So in this case the ordering should be as follows: > > 1. Display: display > 2. Positioning: align-items, position (in any order) > 3. Box model: height, width (in any order) > 4. Colors and typography: background > 5. Other: N/A Done.
	110 position: relative;

	111 display: flex;

	112 align-items: center;

	113 background-repeat: no-repeat;

	114 background-size: cover;

	115 }

	116

	117 .update-graphic-container img

	118 {

	119 display: block;

	120 width: 162px;

	121 height: 162px;

	122 }

	123

	124 .update-graphic-content

	125 {

	126 position: absolute;

	127 top: 65px;

	128 left: 100px;

	129 display: flex;

	130 align-items: center;

	131 }

	132

	133 .version-details

	134 {

	135 margin: 0 1em 0 1em;
	Thomas Greiner 2017/11/14 12:25:42 Well spotted Well spotted
	136 }

	137

	138 .version-details h2

	139 {

	140 margin: 0;

	141 }

	142

	143 .feature-entry

	144 {

	145 display: flex;

	146 align-items: center;

	147 padding: 0 1em;

	148 }

	149

	150 .feature-entry img

	151 {

	152 width: 50px;

	153 height: 50px;

	154 display: block;

	155 margin: 0 1em 0 0;

	156 }

	157

	158 .feature-entry img[dir="rtl"]

	159 {

	160 margin: 0 0 0 1em;

	161 }

	162

	163 #block-ads-text > strong

	164 {

	165 text-decoration: line-through;

	166 font-weight: normal;

	167 }

	168

	169 .store-buttons

	170 {

	171 width: 100%;
	Thomas Greiner 2017/11/14 12:25:41 This causes both scrollbars to be shown when I ope This causes both scrollbars to be shown when I open the page. Note that it's a block element anyway so by default it's already occupying the entire width so there shouldn't be a need for explicitly setting width to `100%`. martin 2017/11/20 11:47:32 Done. Show quoted text On 2017/11/14 12:25:41, Thomas Greiner wrote: > This causes both scrollbars to be shown when I open the page. Note that it's a > block element anyway so by default it's already occupying the entire width so > there shouldn't be a need for explicitly setting width to `100%`. Done.
	172 height: 58px;

	173 margin: 0 0 0 5em;

	174 }

	175

	176 .store-buttons[dir="rtl"]

	177 {

	178 margin: 0 5em 0 0;

	179 }

	180

	181 .store-button

	182 {

	183 display: block;

	184 width: 180px;

	185 height: 58px;

	186 border-radius: 6px;

	187 background-color: #000;

	188 float: left;

	189 margin-bottom: 1em;

	190 }

	191

	192 .store-button[dir="rtl"]

	193 {

	194 float: right;

	195 }

	196

	197 .store-button > img

	198 {

	199 height: 54px;

	200 }

	201

	202 .applestore-button

	203 {

	204 margin-right: 1em;

	205 }

	206

	207 [dir="rtl"] .appstore-button

	208 {

	209 margin-right: 0;

	210 margin-left: 1em;

	211 }

	212

	213 @media(max-width: 960px)

	214 {

	215 #container

	216 {

	217 flex-direction: column;

	218 }

	219 }

	220

	221 @media(max-width: 480px)

	222 {

	223 #content header

	224 {

	225 margin: 2em 0 2em 5em;
	Thomas Greiner 2017/11/14 12:25:42 Detail: This may produce unexpected results for ri Detail: This may produce unexpected results for right-to-left languages. martin 2017/11/20 11:47:33 Previewed it. Saw nothing weird. Let me know if I' Show quoted text On 2017/11/14 12:25:42, Thomas Greiner wrote: > Detail: This may produce unexpected results for right-to-left languages. Previewed it. Saw nothing weird. Let me know if I'm missing something. Thomas Greiner 2017/11/20 17:06:05 I'd have to check whether this issue is visible. B Show quoted text On 2017/11/20 11:47:33, martin wrote: > On 2017/11/14 12:25:42, Thomas Greiner wrote: > > Detail: This may produce unexpected results for right-to-left languages. > > Previewed it. Saw nothing weird. Let me know if I'm missing something. I'd have to check whether this issue is visible. But in general, you use "margin-left" here which will not be applied correctly in right-to-left environments. Therefore we need to handle this even if we don't see anything wrong with it so that it doesn't bite us later on.
	226 }

	227 .column

	228 {

	229 align-items: flex-start;

	230 }

	231 }
	Thomas Greiner 2017/11/14 12:25:41 Coding style: "Newline at end of file, otherwise n Coding style: "Newline at end of file, otherwise no trailing whitespace." See https://adblockplus.org/en/coding-style#general Also applies to other files. Note that you can usually configure this in the text editor settings so that it automatically adds it if it's missing. Same goes for other settings (see https://hg.adblockplus.org/codingtools/file/tip/editorconfig). martin 2017/11/20 11:47:32 Done. Show quoted text On 2017/11/14 12:25:41, Thomas Greiner wrote: > Coding style: "Newline at end of file, otherwise no trailing whitespace." > See https://adblockplus.org/en/coding-style#general > > Also applies to other files. > > Note that you can usually configure this in the text editor settings so that it > automatically adds it if it's missing. Same goes for other settings (see > https://hg.adblockplus.org/codingtools/file/tip/editorconfig). Done.
OLD	NEW

« locale/en_US/updates.json ('K') | « locale/en_US/updates.json ('k') | skin/updates/abp-logo.svg » ('j') | updates.html » ('J')