Work through the human review of the relaunched pages - #129
Merged
Conversation
…r existed Two dead links on /ecosystem. qemu-hisilicon was still addressed to the personal account it was developed in, github.com/widgetii/qemu-hisilicon. It lives under OpenIPC now, and the old URL 404s. telemetry had a card, a stage badge and copy in three languages for github.com/OpenIPC/telemetry, which does not exist and by all appearances never has. Removed, along with proj_telemetry in en, ru and zh. Both were reported rather than found, so there is now a test: every link on a project card must resolve to a repository under the OpenIPC organisation. It is scoped to the cards rather than to every GitHub URL on the page -- the prose links the wiki, which is not a project -- and it was checked against the old link first, where it fails naming widgetii. I checked the other seventeen repositories the page links while I was in there. Those two were the only broken ones.
… is gone Started as a casing inconsistency and turned up two real defects sitting on the same lines. github.com/openipc appeared 24 times against github.com/OpenIPC everywhere else -- in the navbar, the footer, the admin header, the installation note in all three languages, and sixteen route redirects. GitHub does not care about the case, but we should spell our own name one way. Two of those lines were broken rather than untidy. /coupler redirected to github.com/OpenIPC//coupler, with a doubled slash. And /telemetry, in both its plain and its wildcard form, redirected to github.com/OpenIPC/telemetry -- the repository this branch has just removed from /ecosystem, which does not exist and by all appearances never has. /telemetry answers 410 now, next to /binaries and for the same reason recorded there: deleting the route is worse than keeping it, because the catch-all answers unknown paths with a 302 to the homepage, which tells a crawler the page moved there rather than that it is gone. Tests follow each GitHub shortcut, plain and with a deep path, and assert the destination is exactly https://github.com/OpenIPC/<repo>. Checked against the old value first, where it fails naming openipc//coupler.
Homepage - The microline dropped "no cloud required". The lede two lines above already makes that point, and the cloud is not always someone else's -- some of it is ours. - The languages figure is gone from the statistics. Three is not a number worth a tile. - Majestic in the firmware pillar is a link to its wiki page now. The card uses .stretched-link, which covers anything inside it, so the link needed lifting above that overlay; the arrow still takes the whole card. - The partner wall is six labelled rows instead of one block of logos, which is the point of the restructure below. /low-latency - "RunCam and EMAX" becomes "RunCam and other vendors". RunCam is the one actually contributing and the pairing overstated the rest. - RubyFPV and Mario FPV are links, to the same URLs their logos carry. - The dark call-to-action band was butting straight against the white article above it. The gap goes on the page rather than the partial: on the homepage that band follows another dark section, where a top margin shows up as a white stripe. /business - Manufacturers and integrators only. A commercial reader is asking who ships hardware and who installs it, and our code host, our FPV friends and our university teams are not an answer to that. /merchandise - Unlinked from the footer while there is nothing to sell. The route and the page stay: the shop is expected back, plausibly through Open Collective, and deleting them would mean writing it all again. The wall itself - INTERNATIONAL_PARTNERS becomes PARTNER_GROUPS, keyed on why each logo is there: global, manufacturers, integrators, fpv, education, research. Pages ask for the groups they want -- partner_groups(:manufacturers, :integrators) -- and the Russian integrator list still appends to :integrators for :ru only, as before. Not done, and needing a decision rather than a guess: Faceter and rapid have no logo asset in the repository, so neither could be added. Really, TUDSaT and the commented-out Expo Electronica were placed by my reading rather than by instruction -- a sponsor, a student team and a trade show respectively.
At six, half the rows were a single logo under a heading, which reads as a gap rather than a group: research is one logo, and integrators is one until the visitor is Russian. Three rows now, in the order the wall is meant to be read -- who ships and installs the hardware, who hosts us, who we work alongside. English lands at 3/3/6, Russian at 19/3/6 because the integrator list is territory specific. The six groups are untouched. HOME_PARTNER_ROWS only says how the homepage composes them, /business still asks for :manufacturers and :integrators directly, and splitting a row back out is one line. The three labels that now have no row of their own went with it, so the unused-translation count stays where it was.
A universal integrator, so it goes in the :integrators group proper, which every visitor sees, rather than in RU_INTEGRATORS, which is appended only for Russian. The logo is cut from the supplied seeklogo PNG. That file already carries a correct alpha channel with the mark sitting in a band across a square canvas, so the work was cropping to those alpha bounds -- (16,120)-(304,200) -- and scaling the 288x80 result to 580px wide, the width the rest of the wall uses. Nothing was keyed out; an earlier attempt that flattened white to transparent destroyed the alpha the file already had. The supplied JPEG is the same wordmark with the "Know the people around you" tagline underneath, on white. Not used: the tile is 4.5rem tall, where that second line would be unreadable, and every other logo on the wall is a wordmark without one.
Really is an integrator, not a global partner -- it was in :global because its link goes to Open Collective, which said more about how it pays us than about what it does. AnyCam serves more than Russia, so it leaves the territory-gated list for :integrators proper, next to GoodCam, and is now shown to everyone rather than only to Russian-speaking visitors. TUDSaT stays in :education, which was the reading rather than the instruction until now. Expo Electronica is a trade show and belongs in neither. It gets a group of its own, :exhibitions, which no page composes: kept in the system with its logo and link intact, rendered nowhere, ready for the exhibitions page that may come. That is the same treatment /merchandise got, and like that one it needs holding in place -- adding a group to a row is a word. A test asserts it appears on none of the seven pages that carry a wall. RU_INTEGRATORS is down to fifteen and the general integrator row is up to four, so English finally has a row that is not the same three logos.
Homepage: global partners, then manufacturers and integrators, then friendly projects -- and inside that last row, research before FPV before education. HOME_PARTNER_ROWS is a Hash, so both orders are just the order it is written in, and a test asserts the page matches rather than merely contains them. /business already asked for its two groups by name and so already listed manufacturers before integrators. It has a test now saying so, because that order was an accident of the argument list rather than anything stated. An empty row takes its heading down with it. That was already true, but only by luck: partner_rows delegated to partner_logos, where no arguments means every group, so a row naming no groups would have rendered all of them instead of nothing. Both now go through one place that applies the territory rule, and a row with nothing in it returns nothing.
The intro claimed images are removed a couple of days after they arrive. Out, in all three languages, along with the comment that explained how the wording had been softened -- there is no claim left to soften. Nothing about the purge changed: PurgeImagesJob::RETENTION is still two days and the nightly sweep still runs. The page simply stops promising a visitor something it is not in a position to guarantee.
The hero lede says your hardware should not answer to a vendor's cloud, the story heading says cameras should not die when their cloud does, and step 2 was called "The cloud goes dark" -- the same idea three times in the first screen and a half. Step 2 is the one to change: it is about the servers behind the cloud being switched off, which its own body text already says, so the heading can say it too. Two mentions left across the hero and the story, both carrying the argument rather than repeating it.
The dark band ran straight into the text above it on /get-started, the same way it did on /low-latency. Four of the five pages that render it end with an article on white, so the gap is now the band's default rather than something each page has to remember; the fix I put on /low-latency last time is removed, since one mechanism is enough. The homepage is the exception and says so: its band sits directly under a tinted full-bleed section, where a gap is a stripe of white between two coloured bands. It passes flush: true. /donate loses the cryptocurrency card and its copy in all three languages. Open Collective is the whole page now. Both are held by tests -- the band's spacing on the four pages that need it and its absence on the one that does not, and the donation page against the crypto markup and copy coming back.
PR Summary by QodoRefine relaunched pages and restructure partner walls
AI Description
Diagram
High-Level Assessment
Files changed (23)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Everything from the review pass on dev.openipc.org, plus two dead links found
on the way. All of it has been on dev throughout — currently
c026c640, 36page/locale combinations answering 200, zero 5xx.
Homepage
no cloud required. The lede two lines above alreadymakes that point, and the cloud is not always someone else's.
worth a tile.
.stretched-link,whose overlay swallows anything inside it, so the link needed lifting above
that; verified with
elementFromPointthat it is actually clickable.and that step all said "cloud" — three times in the first screen and a half.
manufacturers and integrators, then friendly projects.
/low-latency
RunCam and EMAX→RunCam and other vendors. RunCam is the one actuallycontributing.
/business
Manufacturers, then integrators, each its own labelled row. A commercial
reader is asking who ships hardware and who installs it; our code host and our
university teams are not an answer to that.
/donate, /merchandise, /open-wall
route and the page stay — the shop is expected back.
the purge changed; the page simply stops promising it.
The closing band
It ran straight into the text above it. Four of the five pages that render it
end with an article on white, so the gap is now the band's default rather than
something each page must remember. The homepage passes
flush: true: its bandsits under a tinted full-bleed section, where a gap is a white stripe between
two coloured bands.
The wall itself
INTERNATIONAL_PARTNERSbecomesPARTNER_GROUPS, keyed on why each logo isthere — global, manufacturers, integrators, fpv, education, research,
exhibitions. Pages ask for what they want; the Russian integrator list still
appends to
:integratorsfor:ruonly.globalbecause its link goes toOpen Collective, which said more about how it pays us than what it does.
and is now shown to everyone.
:exhibitions, which no page composes: kept withits logo and link, rendered nowhere, ready for a trade-show page.
An empty row takes its heading down with it. That was already true but only by
luck —
partner_rowsdelegated topartner_logos, where no arguments meansevery group, so a row naming no groups would have rendered all of them.
Two dead links found on the way
qemu-hisiliconon /ecosystem still pointed atgithub.com/widgetii/...,which 404s. It lives under OpenIPC now.
telemetryhad a card, a badge and copy in three languages for a repositorythat does not exist and by all appearances never has. Removed — and
/telemetryanswers 410 rather than redirecting to GitHub's own 404 orfalling through to the catch-all, which 302s to the homepage and would tell a
crawler the page moved there.
github.com/openipcappeared 24 times againstOpenIPCeverywhere else,and
/couplerredirected through a doubled slash. Both fixed.Verification
bin/rails testi18n-tasks missingi18n-tasks unusedNew tests cover the row order and its per-row group order, the empty-row guard,
/business's two rows,:exhibitionsrendering on none of seven pages, theband's spacing on the four pages that need it and its absence on the one that
does not, /donate against crypto returning, and every project card resolving to
a repository under the OpenIPC organisation.
rubocopstill cannot start on this branch —.rubocop.ymlrequiresrubocop-performanceand the Gemfile does not list it. Pre-existing; #120fixed it on master, and this branch predates that merge.
The Chinese copy throughout is mine and has not been read by a native speaker.