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

Split register type column #55

Merged

Conversation

angelina-momin
Copy link
Collaborator

@angelina-momin angelina-momin commented Aug 22, 2024

Solves issues:

  • #89 in splitting register "type" column to "type" and "venue"
  • #85 in changing all URLs to lower case (was going to create a separate PR for that but then I found it easier to implement the changes already since I was redoing a lot of the code in the repo)

Related register PR #98

In the register.csv the column "type" was split into into "type" and "venue" where
Venue = what is in brackets now
Type = outside of brackets

Changes made:

  • Bumped version to 0.7.0
  • Replaced utils_filter_register_table.R with utils_render_register_general.R. The new filtering functionalities utilizes tidyr, dplyr and magrittr functionalities to filter the data which is more efficient than the previous method I used.
  • I also updated the utils for rendering non_register tables to use tidyr, dplyr and magrittr functionalities
  • Removed "Type" and "Venue" column in the venues htmls since it is redundant.
  • Updated config.R's with MD_COLUMNS_WIDTHS, MD_TITLES , FILTER_COLUMN_NAMES, NON_REG_TITLE_FNS, JSON_COLUMNS and more
  • Update utils function documentation and the R markdown files in man

R/utils_filter_register_table.R Outdated Show resolved Hide resolved
R/utils_render_register_mds.R Outdated Show resolved Hide resolved
R/utils_render_register_json.R Outdated Show resolved Hide resolved
R/utils_render_table_venues.R Outdated Show resolved Hide resolved
R/utils_render_table_venues.R Outdated Show resolved Hide resolved
@angelina-momin angelina-momin marked this pull request as draft August 23, 2024 12:23
@angelina-momin
Copy link
Collaborator Author

angelina-momin commented Sep 3, 2024

@nuest I am redoing the code and thought of dropping the "type" and "venue" column for page of registers by venue since those columns are redundant. Tidyr's nest() function also drops the column by which grouping is done so in case of filter by venue it drops the venue column in the resulting nested tables.

Is that okay with you? The json files will only contain the information showed on the page

Refer to the top screenshot for the new look and the bottom screenshot of the old look.

Screenshot 2024-09-03 at 09 43 21

image

@angelina-momin angelina-momin marked this pull request as ready for review September 5, 2024 13:00
@angelina-momin
Copy link
Collaborator Author

angelina-momin commented Sep 5, 2024

@nuest The code is ready for review again. Redid a lot of code on filtering the tables following your suggestion to use tidyr. Please refer to the description of the PR for what I changed.

Can you create a separate issue for the JSON column order please? Prefer a separate issue because this PR already combines two PRs

Copy link
Member

@nuest nuest left a comment

Choose a reason for hiding this comment

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

Wow, what a rewrite. Not easy to digest for me, but I find the new code quite readable. What do you think? Are you happier with the new approach?

Only had a few minor remarks. I'm fine if you go ahead and merge yourself after the final couple of changes, as I might be a bit busy the next days.

Good job!

R/utils_create_filtered_register_csvs.R Outdated Show resolved Hide resolved
R/utils_render_table_venues.R Outdated Show resolved Hide resolved
@angelina-momin
Copy link
Collaborator Author

Wow, what a rewrite. Not easy to digest for me, but I find the new code quite readable. What do you think? Are you happier with the new approach?

Yes I am happy with the new approach. Simpler, easier to read and should be more efficient :)

Only had a few minor remarks. I'm fine if you go ahead and merge yourself after the final couple of changes, as I might be a bit busy the next days.

Good job!

Thanks! Just resolved the comments and i'll proceed with merging

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.

2 participants