Skip to content

G
gitlab-ce
Commit 1df32177 authored by Mike Greiling's avatar Mike Greiling
Browse files
Options

Merge branch 'leipert-proper-icon-validator' into 'master' 

Proper icon validator

Closes #49236

See merge request gitlab-org/gitlab-ce!20620
parents e6b6c7ac 4ff134df
Hide whitespace changes
Inline Side-by-side
Showing with 123 additions and 61 deletions
app/assets/javascripts/vue_shared/components/icon.vue
View file @ 1df32177
<script>
/* This is a re-usable vue component for rendering a svg sprite
icon
Sample configuration:
<icon
name="retry"
:size="32"
css-classes="top"
/>
*/
// only allow classes in images.scss e.g. s12
const validSizes = [8, 12, 16, 18, 24, 32, 48, 72];
let iconValidator = () => true;
/*
During development/tests we want to validate that we are just using icons that are actually defined
*/
if (process.env.NODE_ENV !== 'production') {
// eslint-disable-next-line global-require
const data = require('@gitlab-org/gitlab-svgs/dist/icons.json');
const { icons } = data;
iconValidator = value => {
if (icons.includes(value)) {
return true;
}
// eslint-disable-next-line no-console
console.warn(`Icon '${value}' is not a known icon of @gitlab/gitlab-svg`);
return false;
};
}
/** This is a re-usable vue component for rendering a svg sprite icon
* @example
* <icon
* name="retry"
* :size="32"
* css-classes="top"
* />
*/
export default {
props: {
name: {
type: String,
required: true,
validator: iconValidator,
},
size: {
......@@ -83,6 +99,6 @@ export default {
:x="x"
:y="y"
>
<use v-bind="{ 'xlink:href':spriteHref }" />
<use v-bind="{ 'xlink:href':spriteHref }"/>
</svg>
</template>
app/helpers/icons_helper.rb
View file @ 1df32177
require 'json'
module IconsHelper
extend self
include FontAwesome::Rails::IconHelper
......@@ -38,6 +40,13 @@ module IconsHelper
end
def sprite_icon(icon_name, size: nil, css_class: nil)
if Gitlab::Sentry.should_raise?
unless known_sprites.include?(icon_name)
exception = ArgumentError.new("#{icon_name} is not a known icon in @gitlab-org/gitlab-svg")
raise exception
end
end
css_classes = size ? "s#{size}" : ""
css_classes << " #{css_class}" unless css_class.blank?
content_tag(:svg, content_tag(:use, "", { "xlink:href" => "#{sprite_icon_path}##{icon_name}" } ), class: css_classes.empty? ? nil : css_classes)
......@@ -134,4 +143,10 @@ module IconsHelper
icon_class
end
private
def known_sprites
@known_sprites ||= JSON.parse(File.read(Rails.root.join('node_modules/@gitlab-org/gitlab-svgs/dist/icons.json')))['icons']
end
end
package.json
View file @ 1df32177
......@@ -18,7 +18,7 @@
"webpack-prod": "NODE_ENV=production webpack --config config/webpack.config.js"
},
"dependencies": {
"@gitlab-org/gitlab-svgs": "^1.25.0",
"@gitlab-org/gitlab-svgs": "^1.26.0",
"autosize": "^4.0.0",
"axios": "^0.17.1",
"babel-core": "^6.26.3",
......
spec/helpers/icons_helper_spec.rb
View file @ 1df32177
......@@ -55,6 +55,29 @@ describe IconsHelper do
expect(sprite_icon(icon_name, size: 72, css_class: 'icon-danger').to_s)
.to eq "<svg class=\"s72 icon-danger\"><use xlink:href=\"#{icons_path}##{icon_name}\"></use></svg>"
end
describe 'non existing icon' do
non_existing = 'non_existing_icon_sprite'
it 'should raise in development mode' do
allow(Rails.env).to receive(:development?).and_return(true)
expect { sprite_icon(non_existing) }.to raise_error(ArgumentError, /is not a known icon/)
end
it 'should raise in test mode' do
allow(Rails.env).to receive(:test?).and_return(true)
expect { sprite_icon(non_existing) }.to raise_error(ArgumentError, /is not a known icon/)
end
it 'should not raise in production mode' do
allow(Rails.env).to receive(:test?).and_return(false)
allow(Rails.env).to receive(:development?).and_return(false)
expect { sprite_icon(non_existing) }.not_to raise_error
end
end
end
describe 'file_type_icon_class' do
......
spec/javascripts/ide/components/ide_status_bar_spec.js
View file @ 1df32177
......@@ -70,7 +70,7 @@ describe('ideStatusBar', () => {
status: {
text: 'success',
details_path: 'test',
icon: 'success',
icon: 'status_success',
},
},
});
......
spec/javascripts/ide/components/jobs/detail/description_spec.js
View file @ 1df32177
......@@ -23,6 +23,6 @@ describe('IDE job description', () => {
});
it('renders CI icon', () => {
expect(vm.$el.querySelector('.ci-status-icon .ic-status_passed_borderless')).not.toBe(null);
expect(vm.$el.querySelector('.ci-status-icon .ic-status_success_borderless')).not.toBe(null);
});
});
spec/javascripts/ide/components/jobs/item_spec.js
View file @ 1df32177
......@@ -24,7 +24,7 @@ describe('IDE jobs item', () => {
});
it('renders CI icon', () => {
expect(vm.$el.querySelector('.ic-status_passed_borderless')).not.toBe(null);
expect(vm.$el.querySelector('.ic-status_success_borderless')).not.toBe(null);
});
it('does not render view logs button if not started', done => {
......
spec/javascripts/ide/mock_data.js
View file @ 1df32177
......@@ -74,7 +74,7 @@ export const jobs = [
name: 'test',
path: 'testing',
status: {
icon: 'status_passed',
icon: 'status_success',
text: 'passed',
},
stage: 'test',
......@@ -86,7 +86,7 @@ export const jobs = [
name: 'test 2',
path: 'testing2',
status: {
icon: 'status_passed',
icon: 'status_success',
text: 'passed',
},
stage: 'test',
......@@ -98,7 +98,7 @@ export const jobs = [
name: 'test 3',
path: 'testing3',
status: {
icon: 'status_passed',
icon: 'status_success',
text: 'passed',
},
stage: 'test',
......@@ -146,7 +146,7 @@ export const fullPipelinesResponse = {
},
details: {
status: {
icon: 'status_passed',
icon: 'status_success',
text: 'passed',
},
stages: [...stages],
......
spec/javascripts/jobs/header_spec.js
View file @ 1df32177
......@@ -20,7 +20,7 @@ describe('Job details header', () => {
job: {
status: {
group: 'failed',
icon: 'ci-status-failed',
icon: 'status_failed',
label: 'failed',
text: 'failed',
details_path: 'path',
......
spec/javascripts/jobs/mock_data.js
View file @ 1df32177
......@@ -14,7 +14,7 @@ export default {
finished_at: threeWeeksAgo.toISOString(),
queued: 9.54,
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......@@ -72,7 +72,7 @@ export default {
},
details: {
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......
spec/javascripts/pipelines/graph/job_component_spec.js
View file @ 1df32177
......@@ -10,7 +10,7 @@ describe('pipeline graph job component', () => {
id: 4256,
name: 'test',
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
tooltip: 'passed',
......@@ -65,7 +65,7 @@ describe('pipeline graph job component', () => {
id: 4257,
name: 'test',
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......@@ -111,7 +111,7 @@ describe('pipeline graph job component', () => {
id: 4258,
name: 'test',
status: {
icon: 'icon_status_success',
icon: 'status_success',
},
},
});
......@@ -125,7 +125,7 @@ describe('pipeline graph job component', () => {
id: 4259,
name: 'test',
status: {
icon: 'icon_status_success',
icon: 'status_success',
label: 'success',
tooltip: 'success',
},
......
spec/javascripts/pipelines/graph/job_name_component_spec.js
View file @ 1df32177
......@@ -10,7 +10,7 @@ describe('job name component', () => {
propsData: {
name: 'foo',
status: {
icon: 'icon_status_success',
icon: 'status_success',
},
},
}).$mount();
......
spec/javascripts/pipelines/graph/mock_data.js
View file @ 1df32177
......@@ -13,7 +13,7 @@ export default {
path: '/root/ci-mock/pipelines/123',
details: {
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......@@ -33,7 +33,7 @@ export default {
name: 'test',
size: 1,
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......@@ -58,7 +58,7 @@ export default {
created_at: '2017-04-13T09:25:18.959Z',
updated_at: '2017-04-13T09:25:23.118Z',
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......@@ -78,7 +78,7 @@ export default {
},
],
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......@@ -98,7 +98,7 @@ export default {
name: 'deploy to production',
size: 1,
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......@@ -123,7 +123,7 @@ export default {
created_at: '2017-04-19T14:29:46.463Z',
updated_at: '2017-04-19T14:30:27.498Z',
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......@@ -145,7 +145,7 @@ export default {
name: 'deploy to staging',
size: 1,
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......@@ -170,7 +170,7 @@ export default {
created_at: '2017-04-18T16:32:08.420Z',
updated_at: '2017-04-18T16:32:12.631Z',
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......@@ -190,7 +190,7 @@ export default {
},
],
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......
spec/javascripts/pipelines/graph/stage_column_component_spec.js
View file @ 1df32177
......@@ -7,7 +7,7 @@ describe('stage column component', () => {
id: 4250,
name: 'test',
status: {
icon: 'icon_status_success',
icon: 'status_success',
text: 'passed',
label: 'passed',
group: 'success',
......
spec/javascripts/pipelines/header_component_spec.js
View file @ 1df32177
......@@ -18,7 +18,7 @@ describe('Pipeline details header', () => {
details: {
status: {
group: 'failed',
icon: 'ci-status-failed',
icon: 'status_failed',
label: 'failed',
text: 'failed',
details_path: 'path',
......
spec/javascripts/pipelines/stage_spec.js
View file @ 1df32177
......@@ -20,7 +20,7 @@ describe('Pipelines stage component', () => {
stage: {
status: {
group: 'success',
icon: 'icon_status_success',
icon: 'status_success',
title: 'success',
},
dropdown_path: 'path.json',
......@@ -84,7 +84,7 @@ describe('Pipelines stage component', () => {
component.stage = {
status: {
group: 'running',
icon: 'running',
icon: 'status_running',
title: 'running',
},
dropdown_path: 'bar.json',
......
spec/javascripts/vue_mr_widget/mock_data.js
View file @ 1df32177
......@@ -76,7 +76,7 @@ export default {
path: '/root/acets-app/pipelines/172',
details: {
status: {
icon: 'icon_status_success',
icon: 'status_success',
favicon: 'favicon_status_success',
text: 'passed',
label: 'passed',
......@@ -91,7 +91,7 @@ export default {
name: 'build',
title: 'build: failed',
status: {
icon: 'icon_status_failed',
icon: 'status_failed',
favicon: 'favicon_status_failed',
text: 'failed',
label: 'failed',
......@@ -106,7 +106,7 @@ export default {
name: 'review',
title: 'review: skipped',
status: {
icon: 'icon_status_skipped',
icon: 'status_skipped',
favicon: 'favicon_status_skipped',
text: 'skipped',
label: 'skipped',
......
spec/javascripts/vue_shared/components/ci_icon_spec.js
View file @ 1df32177
......@@ -13,7 +13,7 @@ describe('CI Icon component', () => {
it('should render a span element with an svg', () => {
vm = mountComponent(Component, {
status: {
icon: 'icon_status_success',
icon: 'status_success',
},
});
......@@ -24,7 +24,7 @@ describe('CI Icon component', () => {
it('should render a success status', () => {
vm = mountComponent(Component, {
status: {
icon: 'icon_status_success',
icon: 'status_success',
group: 'success',
},
});
......@@ -35,7 +35,7 @@ describe('CI Icon component', () => {
it('should render a failed status', () => {
vm = mountComponent(Component, {
status: {
icon: 'icon_status_failed',
icon: 'status_failed',
group: 'failed',
},
});
......@@ -46,7 +46,7 @@ describe('CI Icon component', () => {
it('should render success with warnings status', () => {
vm = mountComponent(Component, {
status: {
icon: 'icon_status_warning',
icon: 'status_warning',
group: 'warning',
},
});
......@@ -57,7 +57,7 @@ describe('CI Icon component', () => {
it('should render pending status', () => {
vm = mountComponent(Component, {
status: {
icon: 'icon_status_pending',
icon: 'status_pending',
group: 'pending',
},
});
......@@ -68,7 +68,7 @@ describe('CI Icon component', () => {
it('should render running status', () => {
vm = mountComponent(Component, {
status: {
icon: 'icon_status_running',
icon: 'status_running',
group: 'running',
},
});
......@@ -79,7 +79,7 @@ describe('CI Icon component', () => {
it('should render created status', () => {
vm = mountComponent(Component, {
status: {
icon: 'icon_status_created',
icon: 'status_created',
group: 'created',
},
});
......@@ -90,7 +90,7 @@ describe('CI Icon component', () => {
it('should render skipped status', () => {
vm = mountComponent(Component, {
status: {
icon: 'icon_status_skipped',
icon: 'status_skipped',
group: 'skipped',
},
});
......@@ -101,7 +101,7 @@ describe('CI Icon component', () => {
it('should render canceled status', () => {
vm = mountComponent(Component, {
status: {
icon: 'icon_status_canceled',
icon: 'status_canceled',
group: 'canceled',
},
});
......@@ -112,7 +112,7 @@ describe('CI Icon component', () => {
it('should render status for manual action', () => {
vm = mountComponent(Component, {
status: {
icon: 'icon_status_manual',
icon: 'status_manual',
group: 'manual',
},
});
......
spec/javascripts/vue_shared/components/header_ci_component_spec.js
View file @ 1df32177
......@@ -12,7 +12,7 @@ describe('Header CI Component', () => {
props = {
status: {
group: 'failed',
icon: 'ci-status-failed',
icon: 'status_failed',
label: 'failed',
text: 'failed',
details_path: 'path',
......
spec/javascripts/vue_shared/components/icon_spec.js
View file @ 1df32177
......@@ -10,7 +10,7 @@ describe('Sprite Icon Component', function () {
const IconComponent = Vue.extend(Icon);
icon = mountComponent(IconComponent, {
name: 'test',
name: 'commit',
size: 32,
cssClasses: 'extraclasses',
});
......@@ -30,7 +30,7 @@ describe('Sprite Icon Component', function () {
it('should have <use> as a child element with the correct href', function () {
expect(icon.$el.firstChild.tagName).toBe('use');
expect(icon.$el.firstChild.getAttribute('xlink:href')).toBe(`${gon.sprite_icons}#test`);
expect(icon.$el.firstChild.getAttribute('xlink:href')).toBe(`${gon.sprite_icons}#commit`);
});
it('should properly compute iconSizeClass', function () {
......@@ -50,5 +50,13 @@ describe('Sprite Icon Component', function () {
expect(containsSizeClass).toBe(true);
expect(containsCustomClass).toBe(true);
});
it('`name` validator should return false for non existing icons', () => {
expect(Icon.props.name.validator('non_existing_icon_sprite')).toBe(false);
});
it('`name` validator should return false for existing icons', () => {
expect(Icon.props.name.validator('commit')).toBe(true);
});
});
});
spec/javascripts/vue_shared/components/notes/system_note_spec.js
View file @ 1df32177
......@@ -19,7 +19,7 @@ describe('system note component', () => {
path: '/root',
},
note_html: '<p dir="auto">closed</p>',
system_note_icon_name: 'icon_status_closed',
system_note_icon_name: 'status_closed',
created_at: '2017-08-02T10:51:58.559Z',
},
};
......
yarn.lock
View file @ 1df32177
......@@ -78,9 +78,9 @@
lodash "^4.2.0"
to-fast-properties "^2.0.0"
"@gitlab-org/gitlab-svgs@^1.25.0":
version "1.25.0"
resolved "https://registry.yarnpkg.com/@gitlab-org/gitlab-svgs/-/gitlab-svgs-1.25.0.tgz#1a82b1be43e1a46e6b0767ef46f26f5fd6bbd101"
"@gitlab-org/gitlab-svgs@^1.26.0":
version "1.26.0"
resolved "https://registry.yarnpkg.com/@gitlab-org/gitlab-svgs/-/gitlab-svgs-1.26.0.tgz#d89c633e866d031a9e4787b05eacc7144c3a7791"
"@sindresorhus/is@^0.7.0":
version "0.7.0"
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7