-
Notifications
You must be signed in to change notification settings - Fork 400
Add link to page #483
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
Add link to page #483
Conversation
src/disco/css/App.scss
Outdated
color: #fff; | ||
text-decoration: none; | ||
font-size: 13px; | ||
transition: background .2s; |
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 prefer using 200ms
when it's less than a second, but it probably doesn't matter. I think we wanted the leading zero for decimals, though?
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.
Yep I like the idea of ms for this. I'll change that and we should add that to the style guide when we get around to writing one :)
@pwalm, I'm going to land this so we can get the localizations going. I'll file a new bug for the detailed specs. |
</div> | ||
</header> | ||
{results.map((item, i) => <Addon {...camelCaseProps(item)} key={i} />)} | ||
<a className="cta" href="https://addons.mozilla.org/">{i18n.gettext('See more add-ons!')}</a> |
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 wrote this up yesterday but forget to submit it.
I'm starting to get concerned about our CSS. We are reusing some classes like and cta
looks like it will probably get reused without whoever uses it knowing this style exists. We have some styles on just header
and header h1
too which seems fragile. There are also some styles that are nested 4 levels deep that could likely be reduced to 1 or 2 if we had more specific class names.
I know we discussed using some sort of methodology before and didn't settle on anything but I think we should try and come up with something. London might be a good place to discuss 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.
If we can enforce the system with tooling that'd be great too.
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.
Definitely up for discussing this in London and coming up with a plan - might be better to file an issue with the details you want to cover - as this buried in a merged PR is likely to get lost.
I'll see about de-nesting in the RTL updates.
all: normal: hover: active: copy: |
Add the link to amo as per mozilla/addons#9616

- There was no detailed spec for this but I'm happy to tweak this PR to whatever the details need to be.@pwalm Currently the hover is a lightened version of the background color I grabbed from the docs. Let me know what the exact colours + paddings etc should be and I can update this PR or we can do that separately if you need more time since we need to get this string in to be localized.