Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI - new Icon and Chevron components #6707

Merged
merged 17 commits into from
May 10, 2019
Merged

UI - new Icon and Chevron components #6707

merged 17 commits into from
May 10, 2019

Conversation

meirish
Copy link
Contributor

@meirish meirish commented May 9, 2019

This is a follow-up to #6629.

This PR implements a new Icon component and a variant of the Icon component: Chevron. The new icons use ember-svg-jar to inline SVG files from a set of configured paths. This add-on also gives us a handy ember app that lets us browse all of the discovered SVGs while the development server is running - to see it, run the ember dev server as usual (yarn start from the ui dir) and then navigate to http://localhost:4200/ui/ember-svg-jar/index.html . The new Icon also sources most of the SVGs from HashiCorp's design system, Structure - https://www.npmjs.com/package/@hashicorp/structure-icons .

These replace the usage of the old i-con / ICon component. The old component's implementation relied on using partials to inline the SVGs, which will be deprecated in a future ember release and don't play well with ember-engines. Any SVGs used, but not provided by Structure were renamed to .svg and moved to the public/ folder. The images used for the various types of mounts were moved to public/eco/ - they were previously in /templates/svg/enable/. The rest of the partials in /templates/svg/ were removed.

TODO - this before merging this branch

  • Remove old ICon component
  • Add new components to Storybook
  • Add tests for new components
  • 💃 🕺 👯‍♂ 👯

Following this, I think it makes sense to get this feature branch merged to master, and make progress on the auth/engines settings engine after that (that is the last check item on #6629 's TODO list).

@meirish meirish mentioned this pull request May 9, 2019
4 tasks
align-self: center;
border-radius: 3rem;
background: linear-gradient(135deg, $blue, $purple);
animation: env-banner-color-rotate 8s infinite linear alternate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aww yesss, very excited about this 🌈

Copy link
Contributor

@andaley andaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is coming along so nicely! thanks for the clear write up.

@@ -1,5 +1,5 @@
{{! template-lint-disable no-triple-curlies}}
<div class="console-ui-alert has-text-danger">
{{i-con glyph="close-circled" aria-hidden="true" size=12}}
<Icon @glyph="cancel-circle-fill" aria-hidden="true" />
<pre>{{{content}}}</pre>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the triple curlies necessary here still?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, since this is possibly inserting HTML, we don't want to escape at the template layer. I think the only time this is really used is in the control group flow - if you hit a Control Group, we log the token and a link to the lookup page right in the console.

isButton: false,
glyph: computed('direction', function() {
let { direction } = this;
assert(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woah, i didn't know you could use assert to throw exceptions from outside of a test. very cool!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's a lot like invariant in React land. We don't use it a ton, but they get stripped in prod builds so it's handy. There's some other levels too: https://api.emberjs.com/ember/release/modules/@ember%2Fdebug

@@ -0,0 +1,3 @@
<span class="hs-icon {{class}}" ...attributes>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does hs stand for? hashicorp structure? 😛

Copy link
Contributor Author

@meirish meirish May 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! Perhaps a bit ambitious, but it works for a "namespaced" CSS class - maybe we should go with vlt- 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vlt- is a little more clear to me, but 🤷‍♀

// Set any properties with this.set('myProperty', 'value');
// Handle any actions with this.set('myAction', function(val) { ... });

await render(hbs`{{ic-on}}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is ic-on coming from? I haven't seen it called this anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, ember generators still don't like single-word components, so this was the generated test for icon.js - I'll update all of this before finalizing the PR!


await render(hbs`{{ic-on}}`);

assert.equal(this.element.textContent.trim(), '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also be asserting that the correct classes are applied and aria-hidden is set correctly?

@madalynrose
Copy link
Contributor

This looks great! Super excited about it. bonus points for all those deletions!

@meirish meirish marked this pull request as ready for review May 9, 2019 20:54
@meirish meirish force-pushed the ui-ember-engines branch from ad69f20 to 9bf44b9 Compare May 10, 2019 02:00
@meirish
Copy link
Contributor Author

meirish commented May 10, 2019

OK, sorry for the rebase / force-push, I know that can sometimes lead to weirdness with tracking comments, etc. I rebased the parent branch first, and had to move a few things around to get tests back to 🍏 - that PR is here: #6712.

After that, I rebased this PR on ui-ember-engines and force-pushed, so this PR is now up to date with master and includes the toolbar changes ✨ .

Copy link
Contributor

@joshuaogle joshuaogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I don't see anything off at all, which I know took a lot of work. Thanks for doing this!

@meirish
Copy link
Contributor Author

meirish commented May 10, 2019

Gonna merge and then PR to master! 🎉

@meirish meirish merged commit c440f50 into ui-ember-engines May 10, 2019
@meirish meirish deleted the ui-new-icon branch May 10, 2019 15:32
@meirish meirish mentioned this pull request May 10, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants