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

feat: add nationality to player #44

Closed
wants to merge 1 commit into from

Conversation

MarioRodrigues10
Copy link
Member

No description provided.

@MarioRodrigues10 MarioRodrigues10 linked an issue Sep 15, 2023 that may be closed by this pull request
@MarioRodrigues10 MarioRodrigues10 self-assigned this Sep 15, 2023
@MarioRodrigues10 MarioRodrigues10 added the enhancement New feature or request label Sep 15, 2023
@feliciofilipe
Copy link
Member

feliciofilipe commented Sep 15, 2023

This is not the way to do it, please do not merge it. When I have time I'll explain in more detail the specs of this feature 🙏

@MarioRodrigues10
Copy link
Member Author

I would like to understand better what you meant with the issue then, this is the relation that i understood 🤔

@MarioRodrigues10
Copy link
Member Author

MarioRodrigues10 commented Sep 15, 2023

I would like to know why CI is failling.
This is what returns to me when i run the exact same command locally.
image

@feliciofilipe
Copy link
Member

The problem is not the interpretation of the requirements, is the solution in it itself

@MarioRodrigues10
Copy link
Member Author

I would like to know why CI is failling. This is what returns to me when i run the exact same command locally. image

I needed to update the elixir version due to an error in elixir 1.13 kernel, but I didn't update the actions version simultaneously.

@MarioRodrigues10
Copy link
Member Author

MarioRodrigues10 commented Sep 15, 2023

The problem is not the interpretation of the requirements, is the solution in it itself

What's the problem in the solution, could you review the code ? So i can find where the error is

@feliciofilipe
Copy link
Member

feliciofilipe commented Sep 17, 2023

@MarioRodrigues10 We need to store nationalities as an atom with either a alpha2 or alpha3 standard.

Afterwards we need to use this id to get all the info from a country such as name, nationality, etc.
These information should be in .json files and the name of the files the id of each country.

This also should come with a flag UI component implementation.

@@ -1,2 +1,2 @@
erlang 25.0.2
elixir 1.13.4-otp-25
elixir 1.14.2-otp-25
Copy link
Member

Choose a reason for hiding this comment

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

This should be a separate PR.

Its important to keep changes as short as possible and within the same domain

@@ -5,6 +5,8 @@ defmodule CesiumCupWeb.PlayerLive.FormComponent do
alias CesiumCup.Teams

@extensions_whitelist ~w(.jpg .jpeg .gif .png)
@nationalities File.read!("priv/fake/nationalities.txt") |> String.split("\n")
@positions ~w(goalkeeper fixed wing target)a
Copy link
Member

Choose a reason for hiding this comment

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

This change is out of scope

"#{if @tab == "group" do
"border-quinary text-gray-900"
else
"border-transparent text-gray-500 hover:text-gray-700 hover:border-gray-300"
Copy link
Member

Choose a reason for hiding this comment

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

This change is out of scope

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

Successfully merging this pull request may close these issues.

Add nationality to athlete
2 participants