Skip to content

Update code events page #6158

Merged
roslynwythe merged 4 commits intohackforla:gh-pagesfrom
Anahisv23:update-code-events-page-6082
Feb 1, 2024
Merged

Update code events page #6158
roslynwythe merged 4 commits intohackforla:gh-pagesfrom
Anahisv23:update-code-events-page-6082

Conversation

@Anahisv23
Copy link
Member

Fixes #6082

What changes did you make?

  • Updated the display_object function's if condition to exclude Hack4LA and test objects.

Why did you make the changes (we will use this info to test)?

  • To prevent any events with the "Hack4LA" and "test" title from showing up on the events page.

Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)

Visuals before changes are applied

before 1

before 2

before 3

Visuals after changes are applied

image

after 1
after 2

after 3

@github-actions
Copy link

Want to review this pull request? Take a look at this documentation for a step by step guide!

From your project repository, check out a new branch and test the changes.

git checkout -b Anahisv23-update-code-events-page-6082 gh-pages
git pull https://github.com/Anahisv23/website.git update-code-events-page-6082

Note that CONTRIBUTING.md cannot previewed locally; rather it should be previewed at this URL:

https://github.com/Anahisv23/website/blob/update-code-events-page-6082/CONTRIBUTING.md  

@github-actions github-actions bot added Bug Something isn't working role: front end Tasks for front end developers Complexity: Medium time sensitive Needs to be worked on by a particular timeframe P-Feature: Events https://www.hackforla.org/events/ size: 1pt Can be done in 4-6 hours labels Jan 25, 2024
@njackman-2344 njackman-2344 self-requested a review January 26, 2024 07:02
@njackman-2344
Copy link
Member

Availability: Friday 1/26 5pm - 10pm, Saturday 1/27 3pm-7pm, Monday 1/29 5pm-8pm

ETA: Monday EOD

@roslynwythe roslynwythe self-requested a review January 28, 2024 18:09
@freaky4wrld freaky4wrld self-requested a review January 29, 2024 08:34
@freaky4wrld
Copy link
Member

ETA: EoD 1/29/12
Availability: Evenings

Copy link
Member

@freaky4wrld freaky4wrld left a comment

Choose a reason for hiding this comment

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

Hey @Anahisv23 thanks for taking up the issue, your solution works as required, the content you provided is correct, your branches are correct as well.....

A slight change can be made to simplify the conditional statement..... you can replace

  if (item  && item.project.name (item.project.name !== "Hack4LA" && item.project.name !== "test"))
 

with

if (item?.project?.name !== "Hack4LA" && item?.project?.name !== "test")

Your piece of code is correct too... the above one looks more simpler with optional chaining I've suggested the above line of code from @nelsonuprety1 solution for the same issue.......

I would like to hear from the other reviewers about the above suggestion!!

@nelsonuprety1
Copy link
Member

Yes @freaky4wrld, I also think implementing the optional chaining will make the code look more readable and clean.

@njackman-2344
Copy link
Member

@freaky4wrld @nelsonuprety1 What about deleting the "test mctestyface" and "HackforLA" in the vrms api endpoint itself?
https://www.vrms.io/api/recurringevents

If that's the source of it, delete it there? It's a mess but I thought there's a vs code readable extension for it?

@Anahisv23
Copy link
Member Author

Anahisv23 commented Jan 29, 2024

Hey @freaky4wrld @nelsonuprety1 @njackman-2344 thanks for the feedback and reviewing the PR. I can go ahead and change the conditional statement to incorporate the chaining suggestions. ETA: Monday Jan 29, 3 - 7pm

Updated the if condition to include the chaining suggestion line 150.
@Anahisv23
Copy link
Member Author

Hey y'all, I just changed the if condition to include the chaining suggestions. Let me know if everything looks good.

@Anahisv23 Anahisv23 requested a review from freaky4wrld January 30, 2024 00:34
@freaky4wrld
Copy link
Member

@freaky4wrld @nelsonuprety1 What about deleting the "test mctestyface" and "HackforLA" in the vrms api endpoint itself? https://www.vrms.io/api/recurringevents

If that's the source of it, delete it there? It's a mess but I thought there's a vs code readable extension for it?

Hey @njackman-2344 we use the API to fetch information regarding the events, deleting those events might need some authorization which is given to admins only...... it's better to handle it from our end rather than trying to alter the dependency we are using to get info on those events!!!

@freaky4wrld
Copy link
Member

freaky4wrld commented Jan 30, 2024

Hey @Anahisv23 I can see the instance of Mtesty face maybe it's been updated from Test to Test-Updated Name Prod

Screenshot image
  • Maybe the screenshot looks a bit different because of timezone differences, you can check for the instances of Test-Updated-Name-Prod
  • Also I think we can make use of regex here, and update the current conditional statement to
          if (item?.project?.name !== "Hack4LA" && !/^Test\b/i.test(item?.project?.name))

I would like to know others opinion on the above suggestions...... or @Anahisv23 @roslynwythe @njackman-2344 and @nelsonuprety1 have any other solution to resolve this issue.

@Anahisv23
Copy link
Member Author

Hey @Anahisv23 I can see the instance of Mtesty face maybe it's been updated from Test to Test-Updated Name Prod

Screenshot

  • Maybe the screenshot looks a bit different because of timezone differences, you can check for the instances of Test-Updated-Name-Prod
  • Also I think we can make use of regex here, and update the current conditional statement to
          if (item?.project?.name !== "Hack4LA" && !/^Test\b/i.test(item?.project?.name))

I would like to know others opinion on the above suggestions...... or @Anahisv23 @roslynwythe @njackman-2344 and @nelsonuprety1 have any other solution to resolve this issue.

Hey @freaky4wrld. Good catch! I like this approach. I can update it so it will also handle that new "Test-Updated Name Prod" case.

Copy link
Member

@freaky4wrld freaky4wrld left a comment

Choose a reason for hiding this comment

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

Hey there @Anahisv23 thanks for taking up the issue and bearing with me for making those changes I appreciate your commitment towards the issue....
Also thanks @njackman-2344 @roslynwythe and @nelsonuprety1 for sharing their insights on the issue

These are my observation :

  • to/from branch looks good
  • issue is linked correctly
  • reason for making the changes and the places where changes are made are discussed
  • Visual changes are apt as well
  • Testing on local environment shows no sign of dangling events from the past or the events for testing purposes

PR approved.......

Copy link
Member

@njackman-2344 njackman-2344 left a comment

Choose a reason for hiding this comment

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

I agree with @freaky4wrld for PR approval.

The branch looks correct and issue is linked. Correction made.

Visuals look good and testing as well.

Great!!

Copy link
Member

@roslynwythe roslynwythe left a comment

Choose a reason for hiding this comment

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

Great job on this issue @Anahisv23 - as the other reviewers noted, your initial PR was perfectly fine, but we appreciate your willingness to incorporate their suggestions.

@nelsonuprety1 that was some pretty elegant javascript, thanks for that

@freaky4wrld great review as always, so glad you spotted the test event and proposed a fix.

@roslynwythe roslynwythe merged commit a9e9f11 into hackforla:gh-pages Feb 1, 2024
@Anahisv23
Copy link
Member Author

Hey there @Anahisv23 thanks for taking up the issue and bearing with me for making those changes I appreciate your commitment towards the issue.... Also thanks @njackman-2344 @roslynwythe and @nelsonuprety1 for sharing their insights on the issue

These are my observation :

  • to/from branch looks good
  • issue is linked correctly
  • reason for making the changes and the places where changes are made are discussed
  • Visual changes are apt as well
  • Testing on local environment shows no sign of dangling events from the past or the events for testing purposes

PR approved.......

No problem! Thanks for reviewing the issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working Complexity: Medium P-Feature: Events https://www.hackforla.org/events/ role: front end Tasks for front end developers size: 1pt Can be done in 4-6 hours time sensitive Needs to be worked on by a particular timeframe

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update code behind Events page so events with project names "test" and "Hack4LA" are not displayed

5 participants