Migrate Create newcomers entry page to react #10732
Conversation
There was a problem hiding this comment.
You can definitely use SemUI components here.
There was a problem hiding this comment.
Done, I assume
doesn't have any alternative and hence can be used.
| @@ -169,7 +169,7 @@ export default { | |||
| }, | |||
| [PANEL_PAGES.createNewComers]: { | |||
| name: 'Create New Comers', | |||
| link: createNewComersUrl, | |||
| component: CreateNewComersPage, | |||
There was a problem hiding this comment.
Not 100% sure but I think the correct capitalization is just "Newcomers".
There was a problem hiding this comment.
Oh yes, changed. (there are some places where it's still wrong, I'll change that in a separate PR after this is merged, because those are not related to migration)
app/controllers/admin_controller.rb
Outdated
| competition_ids: params[:competition_ids] || nil, | ||
| ) | ||
| private def competition_list_from_string(competition_ids_string) | ||
| (competition_ids_string || '').split(',').uniq.compact |
There was a problem hiding this comment.
You can make sure that there's always a valid string being passed to this method via stuff like params.fetch(:foo, 'my_default')
| <Header as="h4"> | ||
| <HeaderSubheader> | ||
| Leave blank to check for all competitions | ||
| </HeaderSubheader> | ||
| </Header> |
There was a problem hiding this comment.
Creating a header only for showing a subheader (with no "main" header) smells like bad code to me. What is your goal here, and why is a simple text paragraph not good enough?
There was a problem hiding this comment.
I wanted to show it as 'hint' like thing, so I followed your suggestion in another PR (LINK). Did I implement this wrongly?
| export default function CreateNewComersPage() { | ||
| return ( | ||
| <> | ||
| <Header>Create New Comers</Header> |
There was a problem hiding this comment.
| <Header>Create New Comers</Header> | |
| <Header>Create newcomers</Header> |
danieljames-dj
force-pushed
the
create-newcomers1
branch
4 times, most recently
from
5259a4c to
b13e2ce
Compare
| @@ -169,7 +169,7 @@ export default { | |||
| }, | |||
| [PANEL_PAGES.createNewComers]: { | |||
| name: 'Create New Comers', | |||
There was a problem hiding this comment.
| name: 'Create New Comers', | |
| name: 'Create newcomers', |
This should be changed as well ;-)
There was a problem hiding this comment.
Okay now since you said it, I've changed it :-)
As mentioned in #10732 (comment), I wanted to change them separately all together in another PR after this is merged. For example, the line above the line you highlighted - for that to be changed, I need to change at many places which will make this PR more confusing and those changes will not be aligned to the PR title.
danieljames-dj
force-pushed
the
create-newcomers1
branch
from
b13e2ce to
0c0c0cf
Compare
| @@ -49,7 +49,7 @@ | |||
| All newcomers seem to have been assigned WCA IDs. | |||
| Please <%= link_to "delete the inbox rows", competition_admin_delete_inbox_data_path(@competition, model: :inbox_person), class: 'inbox-trigger btn btn-primary', data: { jquery_method: :delete } %>. | |||
| <% else %> | |||
| Please <%= link_to "assign WCA IDs to newcomers", admin_complete_persons_path(finish_persons_form: { competition_ids: @competition.id }), target: '_blank', class: 'btn btn-primary' %> | |||
| Please <%= link_to "assign WCA IDs to newcomers", admin_complete_persons_path(competition_ids: [@competition.id]), target: '_blank', class: 'btn btn-primary' %> | |||
There was a problem hiding this comment.
Why do you need to wrap this in an array? Passing it as a string should be just fine
There was a problem hiding this comment.
Yes, it do accept single string as well I guess, but there are cases where multiple competitions are also needed to be given as input. So, to make the type of competition_ids as a single type (array of string) instead of multiple type (array of string or single string), I thought of doing this way. Is it fine?
| @@ -1,18 +1,18 @@ | |||
| <% provide(:title, 'Finish unfinished persons') %> | |||
|
|
|||
| <div class="container"> | |||
| <% competition_ids = @finish_persons.competition_ids %> | |||
| <% competition_ids = @competition_ids %> | |||
There was a problem hiding this comment.
This line is redundant now, if you already have a @competition_ids variable then use it everywhere.
There was a problem hiding this comment.
No need to re-assign it to "itself" with another name
There was a problem hiding this comment.
Okay, instead of changing in many places, I thought of doing this way. Changed all the occurrences.
|
|
||
| <h2> | ||
| <%= yield(:title) %> | ||
| <%= link_to "Go back", admin_finish_unfinished_persons_path(competition_ids: competition_ids), class: "btn btn-default" %> | ||
| <%= link_to "Go back", panel_page_path(id: User.panel_pages[:createNewComers], competition_ids: competition_ids), class: "btn btn-default" %> |
There was a problem hiding this comment.
Does this work as expected when competition_ids is an already-split array?
There was a problem hiding this comment.
Thanks for pointing that out, I actually didn't test that portion and realized that it was not even working for single competition ID as it was getting lost during redirection. I've fixed it now.
danieljames-dj
force-pushed
the
create-newcomers1
branch
from
d3cfb9b to
5b3a3a5
Compare
This PR aims at converting the create-newcomers page to react