-
Notifications
You must be signed in to change notification settings - Fork 324
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
feat: pop-up menu and share page tweaks #907
Conversation
Bringing this back into WIP for a few more changes. |
(Note: Some of this work will require changing text on the prefs page -- not doing that in this PR to avoid a merge conflict with prior prefs page changes. It's on my list though!) |
Hi @lidel -- per our conversation, gently pushing this back over in your direction. Thank you for the review! |
This wires up hints to show copied value + adds 30s cache for async lookup function used by this as a small optimization.
f14d83f
to
34c9b0d
Compare
I wired up the hints, they look great! Performance caveat when using without redirectWhen local gateway is used, lookups are cheap, because data loaded in the tab is already on local IPFS Node CID resolution is much more expensive when redirect is off (eg. when embedded js-ipfs is used). This may take from a few seconds to minutes, and means broken state like this (looks offline but in reality it is running): I'll look into ways we can do CID resolution asynchronously, so its lack does ot block UI render. |
This decouples menu render for the time it takes to resolve CID It also improves UX by disabling the 'Copy CID' action until initial resolution is successful
@jessicaschilling I believe I finished implementing missing pieces – please take a quick look if I did not introduce any regression in UI. FYSA I made CID resolution async so if it takes too long it does not block render of browser action popup. Below is example of embedded js-ipfs taking long time to resolve DNSLink + traverse DAG to find CID of (Feel free to tweak the hint for disabled state if needed) |
@lidel I made a small change to the disabled-state message, but LGTM! With the aim of not being too much of a burden to translators due to frequent changes, I added 4 small improvements to this PR. Please feel free to merge if you're OK with these.
|
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 resolved conflicts (preferring versions from this PR) and merged changes from master – might be worth taking a final look at Preferences as its now all up to date with your tweaks @jessicaschilling
Co-authored-by: Marcin Rataj <lidel@lidel.org>
…ipyard/ipfs-companion into feat/popup-header-tweaks
@lidel - Thanks for the resolving the conflicts, and supplying better link-copy text. I think we're good to go here! |
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.
Congratulations, y'all have just made some quick decisions (“Does the global gateway currently have a copy of this IPFS resource? Can I share its global URL in Telegram and expect Telegram to generate a preview under my message? Can I share its global URL in Twitter and expect Twitter to generate a Twitter Card under my message?”) significatly less quicker. This toggle is not even on the first page of the preferences. It becomes necessary to scroll the preferences. |
@Mithgol I make similar checks by opening IPFS resource in Incognito mode. That way you can check how resource loads from public gateway without the need to disable anything in IPFS Companion. For a longer rationale, see my comment at #910 (comment) – hope this helps. |
In brief
A grab bag of small-but-helpful changes to IPFS Companion's pop-up menu and share page. This PR saves height of the pop-up overall and gets closer visually to browser-native menu appearance, but also clarifies usage (particularly for onboarding users) through repositioning/rewording items and adding additional info (values for "Copy" menu items!).
What's changed in the UI
Pop-up header
Pop-up menu
helperText
for all menu items, so it can be used for displaying truncated version of items to be copied (shareable link, content path, CID)helperText
to those items' real values, but doesn't get all the way (see "still missing" below)Share page
Still missing ...And also ...Screenshots
"On" state for a DNSLink page, the most complex display scenario (L-R: Firefox before, Firefox after, Chrome after)
"Off" state (L-R: Firefox before, Firefox after, Chrome after)
Share page (L-R: Firefox before, Firefox after)
Issues resolved/partially resolved
This work draws from a variety of longstanding "discussion thread" issues and will close or partially close the following: