-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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: Use structure-icons as much as possible #6851
Conversation
81fb7d3
to
f849157
Compare
Hey @opihana ! Ok I rebased #6843 onto here which was the fix for the first 2 screengrabs. The other two screengrabs were not changed at all in this PR and have been like that for a while:
I can't find these icons in the Sketch designs anymore, but maybe we should get those back in there (or could you point me to them if they are somewhere I've not seen) and I can follow whatever you do there. We should definitely decide how this entire line should be vertically positioned (P.S. I found an old design with these in, but the design doesn't have the proxy label in which is why I think this troublesome when I first did it)
Actually looking at it more closely, there seems to be a Chrome bug in play here. If you resize the browser window the (i) icon occasionally jumps to be a pixel or so larger than it should be even though no CSS changes. I checked in FF and Safari but it doesn't happen there. Looks to be a little browser bug, but I'd probably look at that in a separate PR if at all. |
6830f7c
to
3e381a7
Compare
f849157
to
2e4983a
Compare
Codecov Report
@@ Coverage Diff @@
## ui-staging #6851 +/- ##
==============================================
- Coverage 65.64% 65.61% -0.03%
==============================================
Files 443 443
Lines 53312 53312
==============================================
- Hits 34994 34981 -13
- Misses 14095 14100 +5
- Partials 4223 4231 +8
Continue to review full report at Codecov.
|
02e0ab4
to
ccf85bb
Compare
c3d2fe1
to
b330b80
Compare
1a1a7a4
to
a982da9
Compare
|
a982da9
to
fe03fc6
Compare
This has been rebased and is ready again for review. We also added one last commit (now that we have rebased from other feature work) to remove the remaining CSS in our app level
|
e38928a
to
aa680d5
Compare
fe03fc6
to
77984c5
Compare
@johncowen Does the preview link in the original comment stay valid? If so, I noticed that there are a few issues: There's no content on the Settings page And the intentions were doing this weird swapping of the name when I click on it. Watch carefully! |
77984c5
to
2548aea
Compare
Rebased #6965
This is a mix of our mocks and ember:
As point 1 is unlikely to happen in production you are unlikely to see this in reality (there is a chance that the intention could be edited by other means before you've clicked on it, therefore you would see this, but if this does happen this problem is has been in the UI since the start and I'd guess we are unlikely to look into it - one suggestion would be that we use something other than ember-data to see if that resolves the issue. I'm not sure if this exact thing is inherent with ember or ember-data. P.S. Yeah the preview link does stay valid! 😄 All in all this PR only has CSS changes (and one tiny HTML change) just to replace icons, so anything javascript-y like this won't have been added via this PR. |
2548aea
to
a617d72
Compare
@opihana are you ok to re-review this, or would you rather I add someone else to review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion, but not a required action. 😸
a[rel*='external'] { | ||
@extend %with-exit; | ||
a[rel*='external']::after { | ||
@extend %with-exit-icon, %as-pseudo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find %with-exit-icon
. Is it stored in app/styles/base/icons files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found. Please disregard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You make a good point here though. If we imported base/icons
here it would provide a clue for where the %with-exit-icon
is coming from, even though its not required (we import it higher up at a more global level). I've tried to do that in other areas but as I'm not 100% on whether we are ok relying on global imports I've not been entirely consistent. Definitely something we can look at and both decide upon though 👍
@@ -1,3 +1,10 @@ | |||
/*TODO: Rename this to %app-view-brand-icon or similar */ | |||
%with-external-source-icon { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we extend the with-external-source-icon
that already exists and leave the TODO? I suggest this since there's no change between this one and the one that already exists, for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I follow what you are saying here, that would be a great idea for if there were other places that we don't own that were using these CSS files:
%app-view-brand-icon {
@extend %with-external-source-icon;
}
then both placeholders would do the same thing, and we could gradually deprecate the old one.
As we own the entire source here it probably just as good to search/replace the instances where we use %external-source-icon
with the new name, might be something you could PR if you fancied it?
All that ^ is if I've understood correctly - we can go over it later.
This PR removes as many as our old icons as possible, in favour of
structure-icons
, and is partly as a result of working too far on icons during #6639.Preview Link
Once both #6639 and this PR is merged down, we should be 100% using our
base
CSS - which is almost 100%structure-icons
. For outstanding icons that we have inbase
but not instructure-icons
we'll work to get them integrated.Following merge of this and #6639 we should be able to get rid of our project
components/icons/index.scss
file 🎉P.S. This makes use of CSS
mask-image
which has no IE11 support. There is no polyfill included here.