-
Notifications
You must be signed in to change notification settings - Fork 43
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
Offramp link to docs #240
Offramp link to docs #240
Conversation
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 need to test that your deliverables work before you ask for reviews. It's not a link. It doesn't work.
Sorry about that my bad. I Assumed it was going to link and rushed through it. So sorry |
Crazy haven't experienced this. What size of screen are you viewing this on? |
You will notice it after wrapping the div, I was just highlighting it |
static/index.html
Outdated
@@ -182,7 +182,7 @@ | |||
<div id="build"> | |||
<a target="_blank" rel="noopener noreferrer"></a> | |||
</div> | |||
<div class="faq-icon" href="https://github.com/ubiquity/work.ubq.fi/discussions/53" target="_blank"> | |||
<div class="faq-icon" onclick="window.open('https://github.com/ubiquity/work.ubq.fi/discussions/53', '_blank')"> |
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.
wrap the div in an <a>
instead of this
edit: why are you using this approach, is there a specific reason?
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.
Well I did this just because I was wrapping it with a div and you can href on a div
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.
Okay, I think using a proper anchor tag and then wrapping the svg in a div to style it like @0x4007 suggested is the way to go.
It's not a huge SEO factor in this case but it's a better practice to use <a>
where applicable
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.
Okay thanks. Corrected to reflect this. I thought @0x4007 meant to get rid of the <a>
and wrap it just with a <div>
. My bad!
lmao I've been adding comments and seeing I realise my mistake now, @jordan-ae can you see my comments? 😂 |
I have wrapped the div but the alignment doesn't break. Any steps to repro this? |
other than an unstyled |
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.
Looks good.
Unable to repro misaligned notifications now. That may have been my own error my mistake @jordan-ae.
remember to hit "resolve conversation" if you have implemented the change appropriately and there are no open questions to/from the reviewer for that request.
You should match the styles of the |
Fixed it the styles now match the build |
@jordan-ae As far as I understand the commit hash should be horizontally aligned with the question sign, right now they're not on the same level, pls fix |
Looks aligned when I pull locally on both mobile and desktop, notifications don't appear to be out of alignment either. @jordan-ae for future aesthetic changes it would be nice if you could show a video of mobile and desktop or a screenshot of both., I would appreciate it and I try to do the same in my PRs |
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.
thanks @jordan-ae
Edit:
Just saw CI is failing, let's get that resolved
They are passing locally, so how about I fix CI in another pull? hold tight
They are working locally too for me. Maybe we can re-run the workflow to rule out the possibility of an inconsistency happening during runtime |
@jordan-ae could you merge jordan-ae#1 please |
CI: Test timeout increase
@jordan-ae can you resolve the conflict please and we can get this merged. my ci run passed so we should be good to go |
Co-authored-by: Keyrxng <106303466+Keyrxng@users.noreply.github.com>
Co-authored-by: Keyrxng <106303466+Keyrxng@users.noreply.github.com>
@0x4007 I think this could be merged now if you are also happy with these changes |
static/scripts/rewards/render-transaction/render-transaction.ts
Outdated
Show resolved
Hide resolved
static/scripts/rewards/render-transaction/render-transaction.ts
Outdated
Show resolved
Hide resolved
@0x4007 i think this is ready to be merged. Please 🙏 |
Actually @Keyrxng you're the last say on this repo |
Yeah I definitely want direct say in cosmetics. Thanks. Also the other approvals aren't explicitly required but it's generally a good idea to get more approvals. |
Resolves #239