Migrate Records Page to React #10623
Conversation
|
|
|
|
|
Added mixed history and history This means this is now feature complete, but there is definitely possibilities of refactors as there is a lot of code reuse |
|
Did a lot of refactoring to reduce code reuse |
| useEffect(() => { | ||
| window.history.replaceState(null, '', recordsUrl(event, region, gender, show)); | ||
| }, [event, region, gender, show]); |
There was a problem hiding this comment.
When I navigate around with browser back/forward, I get crashes.
I assume this affects the other migration PRs which handle this the same way.
There was a problem hiding this comment.
It does not do that for me... window.replaceState should just replace the browser bar and not add an entry to it
There was a problem hiding this comment.
Sometimes the hot reloading that we enabled semi-recently (like, a few months ago) creates inconsistent state updates like that. Is the error reproducible for you?
There was a problem hiding this comment.
I opened a fresh local host tab, navigated to my competitions, navigated to records, clicked back (and got to my competitions), clicked forward, and got the error again.
There was a problem hiding this comment.
still can't reproduce
app/webpacker/components/Results/Records/SeparateRecordsTable.jsx
Outdated
Show resolved
Hide resolved
| import { Table } from 'semantic-ui-react'; | ||
| import I18n from '../../lib/i18n'; | ||
|
|
||
| const headerConfig = { |
There was a problem hiding this comment.
Slightly confused why there are 4 configs and 4 header components but 5 'show categories'. Probably mixed-history re-uses either mixed or history, but it's not immediate to trace down.
There was a problem hiding this comment.
Yes, history and mixed history have the same headers, just with one more column for events so it has the conditional rendering
There was a problem hiding this comment.
Chiming in to note that I'd be happy to see a short comment explaining the Array(4).fill('')s. (Probably one comment to explain all of them is enough)
There was a problem hiding this comment.
Can we instead do a single header cell of width 5 (or 3 when appropriate)? FM results don't look nice:
| ); | ||
| } | ||
|
|
||
| function TableWrapper({ competitionsById, rows, show }) { |
There was a problem hiding this comment.
I don't like how half of the show-based logic is handled here, and half is handled in the children i.e. in RecordsTable. Handling it all in one spot/level is much nicer. And the higher that level is (and the 'dumber' the children are) the better, generally.
Here, it would be best to either have 5 cases/returns here (1 per show, and if some of those components are very similar then they can just reuse common children), or replace the show prop in RecordsTable with resultsAreGroupedByEventId={show !== 'mixed history'} and other props, so that RecordsTable isn't directly aware of show. The latter looks complicated/messy in this case, so I'd go with the former. (I'd be happy to directly contribute commits to the PR if that would be helpful.)
There was a problem hiding this comment.
Feel free to commit to it!
There was a problem hiding this comment.
I raised a PR, since I couldn't figure out how to push directly.
There was a problem hiding this comment.
took me a while, but merged.
Can you not just do
gh pr checkout 10623
and then push to the PR?
| @record_timestamp = ComputeAuxiliaryData.successful_start_date || Date.current | ||
|
|
||
| respond_to do |format| | ||
| format.html {} | ||
| format.json do | ||
| cached_data = Rails.cache.fetch ["records-page-api", *@cache_params, @record_timestamp] do | ||
| rows = DbHelper.execute_cached_query(@cache_params, @record_timestamp, @query) | ||
| comp_ids = rows.map { |r| r["competitionId"] }.uniq | ||
| if @is_slim || @is_separate | ||
| rows = compute_slim_or_separate_records(rows) | ||
| end | ||
| competitions_by_id = Competition.where(id: comp_ids).index_by(&:id).transform_values { |comp| comp.as_json(methods: %w[country], include: [], only: %w[cellName id]) } | ||
| { | ||
| rows: rows.as_json, competitionsById: competitions_by_id | ||
| } | ||
| end | ||
| render json: cached_data | ||
| end | ||
| end |
There was a problem hiding this comment.
This snippet should be merged/extracted to a common function once the Rankings page gets merged.
| import { Table } from 'semantic-ui-react'; | ||
| import I18n from '../../lib/i18n'; | ||
|
|
||
| const headerConfig = { |
There was a problem hiding this comment.
Chiming in to note that I'd be happy to see a short comment explaining the Array(4).fill('')s. (Probably one comment to explain all of them is enough)
| if (region !== 'world') { | ||
| queryParams.append('region', region); | ||
| } | ||
|
|
||
| if (gender !== 'All') { | ||
| queryParams.append('gender', gender); | ||
| } | ||
|
|
||
| if (eventId){ | ||
| queryParams.append('event_id', eventId) | ||
| } | ||
|
|
||
| if (show){ | ||
| queryParams.append('show', show) | ||
| } |
There was a problem hiding this comment.
See my comment on the Rankings PR about redundant-but-not-redundant parameters ;)
because it doesn't wrap the table
Still need to do some of the other table types, but it's very similar to the rankings page
