Migrate Competition Overview Page to React #10418
Conversation
|
Migrated all information over. Now onto the styling |
|
Moving it to the grid with the right sizes was quite easy actually. But I don't really know what the correct equivalent of a Descriptive List is in Semantic |
|
I can rebuild it with another grid, maybe that looks good enough |
I was able to replicate it with a grid, but I did have to set the padding to 0 manually
|
Almost everything moved over to grid
|
FinnIckler
force-pushed
the
react/tabs
branch
from
226ea43 to
3c2f74e
Compare
|
|
||
| export default function CompetitionSeriesRequirement({ competition }) { | ||
| return ( | ||
| <div> |
There was a problem hiding this comment.
Can this be an empty <> fragment tag?
| <ul> | ||
| {competition.series_sibling_competitions.map((comp) => ( | ||
| <li key={comp.id}> | ||
| <a href={`/competitions/${comp.id}`}>{comp.name}</a> | ||
| </li> | ||
| ))} | ||
| </ul> |
There was a problem hiding this comment.
This should definitely be a SemUI <List>
| competition.currency_code, | ||
| ), | ||
| })} | ||
| <br /> |
There was a problem hiding this comment.
Not a big fan of br'ing without knowing what comes next. What problem are you trying to solve?
There was a problem hiding this comment.
the <br /> are from the original file
| ) : competition['some_guests_allowed?'] && ( | ||
| I18n.t('competitions.competition_info.guests_free.restricted') | ||
| )} | ||
| <br /> |
| {competition.competition_events.map((event) => (event['has_fee?'] ? ( | ||
| <dl key={event.id}> | ||
| <dt>{event.event.name}</dt> | ||
| <dd>{isoMoneyToHumanReadable(event.fee)}</dd> | ||
| </dl> | ||
| ) : null))} |
There was a problem hiding this comment.
As it currently stands, you're opening an entirely new list (<dl> tag) per event. Shouldn't it be an outer dl tag where one pair of "dt+dd" tags is added as list entry per event?
There was a problem hiding this comment.
this is just not migrated yet!
There was a problem hiding this comment.
Lots of dangling br, won't comment on all of them.
Not sure what you're trying to solve, but a simple <List> (which does not have enumeration bullet points by default in SemUI!) might already do the trick in terms of line breaks.
| const updatePath = (tabSlug) => { | ||
| window.history.replaceState({}, '', `${window.location.pathname}#${tabSlug}`); | ||
| }; |
There was a problem hiding this comment.
Do we have shared lib functions for this? I feel like one or two other parts of our frontend code use similar features already
There was a problem hiding this comment.
Not happy about the repetitive Gridding in here. Have you (a) tried out SemUI List using List.Header, List.Description and perhaps even List.Icon and (b) if a didn't work, can you please move the Grid cells into an "internal" component that you can reuse at least within this page?
Currently wip porting the registration requirements over
There is an issue that it only does one markdown request at once, but a bunch of fields are in markdown... so not sure what to do about that.
Progress:
