Skip to content
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

Migrate some parts of persons page to React #9184

Draft
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

simonkellly
Copy link
Member

@simonkellly simonkellly commented Apr 1, 2024

This will use react to display the Records and soon the Championship Podiums also when react is among the url params

@simonkellly simonkellly changed the title Migrate the persons page to React Migrate some tabs of persons page to React Apr 1, 2024
@simonkellly
Copy link
Member Author

I think the best way to split this migration up is likely to do:

  • all of the relatively static tables in one pr
  • big list of all results in another pr
  • map in a 3rd PR (unless its trivial, but I have not investigated that yet)
  • Final pr to turn on the changes for everyone

So this will be first one of those

@gregorbg
Copy link
Member

gregorbg commented Apr 1, 2024

Just as a short remark (haven't looked at your code in detail):
I think it will be easier if you add a "secret" query param, and if that query param is set/toggled, you go full-on React.

Don't inject mutliple react_components in the ERB template here and there. You could rename the existing layouts page to "show_legacy" and have show.html.erb basically be:

<% if new_design_toggled %>
  <%= react_component('blabla') %>
<% else %>
  <%= render 'show_legacy' %>
<% end %>

...where the new React component is its own standalone thing that is incomplete / WIP at the beginning of the migration process.
Right now it might not seem like much, but at least during the second or third PR this will make live much easier for you when coupling the individual parts together.

@simonkellly simonkellly changed the title Migrate some tabs of persons page to React Migrate some parts of persons page to React Apr 7, 2024
@simonkellly
Copy link
Member Author

I have finished migrating the static parts (plus map).

What is left is the results for event table, and tying it together (as mentioned above) which will be a seperate pr.

There is some logic in the view files such as in the _details.html.erb file which handles the serialization for the react component. I think leaving this here is the best option for now, as it is likely parts of the data needed will be reused across different sections of the page, which will eventually all be rendered under one react component, but I am open to feedback on this!

@simonkellly simonkellly marked this pull request as ready for review April 7, 2024 21:32
@simonkellly
Copy link
Member Author

In case anyone wants to see different features, I have been testing with various different people namely:

2012BEAH01 - Lots of records, but still some stuff on the profile missing.
2012PATE01 - Has some PRs which were from a previous country
2015YAKH01 - Has multiple previous countries

@gregorbg
Copy link
Member

gregorbg commented Apr 7, 2024

Can I haz screenshot plz? :D

(I am thrilled to see the progress but I can't boot my computer right now)

@gregorbg
Copy link
Member

gregorbg commented Apr 7, 2024

Also, why did you decide to glue smaller React components into all the different legacy views (patchwork-style) instead of one top-level "If flag, then show THE react page (which may be incomplete and still missing some sections entirely) or else show legacy"?

I don't want to sound very strict or dictate your programming style, so if you feel comfortable with your current approach then okay. My thinking is just that by replacing bit by bit with smaller components (your current approach) limits you too much in terms of layout and design. It ties you to the exact layout of the legacy page.

The alternative would be to have one global React component that is still "work-in-progress" (compared to the old legacy page) and grows as you go. This gives you more degrees of freedom, like a "blank canvas" to freely think about the best approach for arranging the individual sub-sections

@simonkellly
Copy link
Member Author

I didn't really think about having it in one big page with missing parts (sorry if I misunderstood what you meant in your message last week), but happy to do so.

In terms of the layout, its almost the exact same as before but with semantic ui. I don't have any ideas specifically about ways it could be made different, but its basically the same work to make those changes down the line vs now. If you have any suggestions in this department I would definitly like to hear them.

image

@gregorbg
Copy link
Member

gregorbg commented Apr 8, 2024

I didn't really think about having it in one big page with missing parts (sorry if I misunderstood what you meant in your message last week), but happy to do so.

I think the most important point is to find an approach that works well for you. Don't sink several hours into this refactor if it's inconvenient for you to work with and slows down your progress significantly. In my opinion, having one big page has the following advantages:

  1. It more closely mirrors the "end result", that is when we fully replace the page it will be one central entry point to one (then fully migrated) React component.
  2. It prevents you from having to pass small chunks of data around here and there. You can already start thinking about efficiently "bundling" the data, and avoid redundantly passing the same bits of data around to all the smaller components again and again
  3. (Arguably the biggest advantage) It removes the restraints of the existing layout, giving you more degrees of freedom to play with the layout.

But again, this is just my perspective and you are totally free to disagree. I won't block the merge with your current "bite-sized components" approach and the emphasis is for you to work in a way that feels most efficient to you.

In terms of the layout, its almost the exact same as before but with semantic ui. I don't have any ideas specifically about ways it could be made different, but its basically the same work to make those changes down the line vs now. If you have any suggestions in this department I would definitly like to hear them.

I don't have any specific directions or reference material that I would like you to implement. But there are so many small ideas that immediately come to my mind.

  • "Medal Collection" and "Record Collection" can use icons and colors. Like rendering the counts actually in gold/sivler/bronze font color, or a "winner's podium" or "winner trophy" icon next to the championship podium.
  • Records can be made a "collapsible" extension of the "Record collection" block
  • Championship Podiums can be made a "collapsible" extension of the "Medal collection" block
  • The Region/ WCAID/ Gender/ Competitions/ Solves statistics can be put into a vertical list next to the profile picture. Play around with https://react.semantic-ui.com/views/card/ just for the fun of it
  • Do we want to more prominently place the map??

As previously stated, none of these are requirements or binding directives. They are really just random ideas and my point is to "free your mind" from the restraints of the previous layout (which I think is the easiest when you have one big work-in-progress React component that you can freely play around with). Are you fully happy with the page layout of the current page? Is there really nothing you had always wished would be laid out differently / more convenient? Now is the chance to change it!

Comment on lines 42 to 44
{person.name + (canEditUser ? ' ' : '')}
{canEditUser && (
<a href={editUrl}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{person.name + (canEditUser ? ' ' : '')}
{canEditUser && (
<a href={editUrl}>
{person.name}
{canEditUser && (
{' '}<a href={editUrl}>

import { events } from '../../lib/wca-data.js.erb';
import { AttemptItem } from './TableComponents';

function groupByEvent(results) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lodash has a groupBy function; it's used in OfficersAndBoard for example.

Comment on lines 58 to 62
const allEvents = events.official.map((event) => event.id);
Object.entries(events.byId).forEach((entry) => {
if (allEvents.find((e) => e === entry[1].id)) return;
allEvents.push(entry[1].id);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be possible to do this with filters/maps, which I think would be better than modifying const arrays.

@simonkellly
Copy link
Member Author

image

I am looking at this kind of direction for the top of the page to replace the current image and single table row.

This is a very rough draft but just wanted to put it here anyway (It is taking a while for me to get to grips with how semantic-ui handles grids and layouts etc.)

@simonkellly
Copy link
Member Author

As a bit of an update,
I spent some time working on this though the extremely large amount of variance within the images used for avatars makes it so that having the image displayed on its own as in the currently layout is almost definitely the best option (unless we were to restrict avatar sizes more which is a lot of unnecessary work).

Hopefully I will have a nice demo image to show in a few days

@simonkellly simonkellly marked this pull request as draft June 13, 2024 18:29
@simonkellly
Copy link
Member Author

simonkellly commented Jun 13, 2024

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants