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

Display primary nationality #10

Merged
merged 12 commits into from
May 9, 2023
Merged

Conversation

Ayedrian
Copy link
Contributor

The way the emojis are resized is not ideal with some rather ugly inline CSS "hacks", other than that displaying nationality in both the small card as well as the detailed view is implemented.

@Ayedrian Ayedrian requested review from Mtze and weinhuber April 19, 2023 14:11
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The absolute value for the top margin is not elegant but without this the emoji moves vertically down since the font size has been increased for readability and overall improved look. There's definitely a nicer way to do it but I'm not sure how at the moment, I'm open to suggestions, thanks in advance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the badge class seems like a "necessary evil" to get the text displayed to the right of the badges again (previously there was a bug that the text floated onto the next line below, which caused it to render behind content that was actually supposed to be on the next line). Removing the badge class oddly fixes this behavior, but the gray background associated with badges is of course gone.

@Ayedrian Ayedrian linked an issue Apr 19, 2023 that may be closed by this pull request
@Ayedrian Ayedrian changed the title Feature/display primary nationality Display primary nationality Apr 20, 2023
Copy link
Member

@Mtze Mtze left a comment

Choose a reason for hiding this comment

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

Ideally you implement a hover tooltip which shows the Countrycode of the flag in the overview page (I case somebody does not recognise the flag)

In the detail view you should definitely display the Country Code next to the flag

@Mtze Mtze added enhancement New feature or request prio:high labels Apr 27, 2023
@Ayedrian Ayedrian added the ready to review Indicate that reviewer should have a look label Apr 27, 2023
@Ayedrian Ayedrian requested a review from Mtze April 27, 2023 22:48
@Ayedrian
Copy link
Contributor Author

What changed: A new attribute in the CSV (a person's primary nationality, specified using ISO 3166-1 alpha-3) is displayed using the corresponding flag (emoji) in the preview card and using the flag as well as country code in the detailed person view.

Why the changes: A functional requirement was requested that instructors be able to see a person's primary nationality.

Testing steps: Start up a container using the README instructions, distribute the people in the pool according to some constraints of your choice, confirm that the emoji flags are being displayed in the preview cards and that hovering over them shows the country code, click on a person's card to open the detailed view and confirm that both the flag as well as the country code are listed.

Copy link
Member

@Mtze Mtze left a comment

Choose a reason for hiding this comment

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

Changes seem to work with the example data - As soon as we have a working csv import again we can test it with real data

@Mtze Mtze merged commit 3c83420 into develop May 9, 2023
@Mtze Mtze deleted the feature/display-primary-nationality branch May 9, 2023 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request prio:high ready to review Indicate that reviewer should have a look
Projects
None yet
2 participants