-
-
Notifications
You must be signed in to change notification settings - Fork 73
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
[Feature] Implements close icon for vocabulary #311
[Feature] Implements close icon for vocabulary #311
Conversation
✅ Deploy Preview for vocabulary-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/css/library-vars.css
Outdated
--icon-sprite: var(--fa-close); | ||
} | ||
|
||
.icon-replace.fa-instagram { |
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.
This appears to be erroneous?
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.
@possumbilities my thought was to accomodate for both an .icon-attach
and an .icon-replace
instance. But if the .icon-attach
usecase is not necessary, I can remove it
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 meant it looks like line 237
is a duplicate of fa-instagram
even though this PR is for fa-close
?
this section is a duplicate of line 249
as it currently is
and line 261
already has an fa-close
for replacing?
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.
Aha! I see the issue. fa-instagram
was duplicated. That was a mistake.
The for closing, i would remove the .icon-attach
implementation inline 233,
since a single .icon-replace
implementation would suffice. I had in mind that having the two would give flexibility for any contibutor that might want to implement it
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.
it's fine to have attach and replace for fa-close. It was just the instagram
duplication mistake that needs addressing :)
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.
Mr Quick fingers has removed the attach implementation 🥲.
I have removed the duplicate instrgram
implementation
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.
all good now :)
Fixes
Description
This PR adds a close icon to to the vocabulary source files.
Technical details
The SVG implementation was gotten from the official font awesome website, then resized to be compatible with the existing
viewBox="0 0 30 30"
property.Tests
<span class="icon-replace fa-close">Close</span>
-Observe the close icon displays properly
Checklist
Update index.md
).main
ormaster
).visible errors.
Developer Certificate of Origin
For the purposes of this DCO, "license" is equivalent to "license or public domain dedication," and "open source license" is equivalent to "open content license or public domain dedication."
Developer Certificate of Origin