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

Add city council mtg zoom link URL #351

Merged
merged 6 commits into from
Nov 16, 2023

Conversation

jerrycyip
Copy link
Collaborator

@jerrycyip jerrycyip commented Nov 7, 2023

📜 Description

  • Describe the general shape of this PR (new feature? refactor? bug fix? one-line change?)

    • Per business owner clarifications from Ellina Yin, added a configurable constant to use as the default zoom url link for city council meetings: 'https://sanjoseca.zoom.us/j/88471470054'. Prior to changes, there was no meeting zoom url link to redirect users and the application simply opened another tab with the same ATM application page.
  • Describe what changes are being made

    • Per business owner clarifications from Elina Yin, added a configurable constant to use as the default zoom url link for city council meetings: 'https://sanjoseca.zoom.us/j/88471470054'.
    • As part of the changes also removed the button element tag which was originally inside of the zoom url anchor tag as this is poor practice from a non-semantic HTML perspective and is likewise not good for accessibility (A11) purposes. To retain the same design, I instead applied the button styling to the zoom anchor tag itself. In addition, I fixed the anchor tag's styling such that the clickable area of the anchor tag only extends up to the width of the element instead of the entire width of the container (see short video below for demo of before/after).
  • Describe why these changes are being made

    • See above
  • List the use cases and edge cases relevant to this PR

    • See above
  • Describe how errors will be handled. How will we know if this code breaks in production

    • N/A
  • Describe any external libraries/dependencies added or removed in this PR

    • N/A
  • Describe any security risks are there regarding this change

    • N/A
  • Describe how you tested these changes

    • Tested before and after changes in front-end browser.
  • Link to relevant external documentation


📋 Mandatory Checklist

  • Example of a checked item (please remove when creating your Pull Request)

  • Linked to the Github Issues being addressed using the right sidebar ➡️

  • Have you discussed these changes with the project leader(s)?

  • Do all variable and function names communicate what they do?

  • Were all the changes commented and / or documented?

  • Is the PR the right size? (If the PR is too large to review, it might be good to break it up into multiple PRs.)

  • Does all work in progress, temporary, or debugger code have a TODO comment with links to Github issues?

  • If you changed the user interface, did you add before and after screenshots to below?


🖼️ Screenshots and Screen Recordings

Before

pre_change_behavior.mov

After

post_change_behavior.mov

📘 Glossary

  • PR = Pull Request

Per business owner clarifications added configurable constant to use as
default zoom url link for city council meetings:
'https://sanjoseca.zoom.us/j/88471470054'
Copy link
Contributor

@JMStudiosJoe JMStudiosJoe left a comment

Choose a reason for hiding this comment

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

Updating to env car would be best if possible at this time, if not let me know in response to comment here please.

@@ -0,0 +1,6 @@
// City Council Meeting Zoom Link URL
const MeetingZoomURL = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add to the env vars that way easier to update if needed

Copy link
Collaborator Author

@jerrycyip jerrycyip Nov 15, 2023

Choose a reason for hiding this comment

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

Appreciate the review and suggestion! Accordingly, I've moved this to an environment variable as per latest commit: "1fca7278601741cf38ad9f51d6e959845d21d9bb". It took some trial and error before I remembered that create-react-app requires environment variables must have the prefix "REACT_APP_" so I also added a comment in the environment file as a reminder.
(side note: I also merged in the recently approved PR 350 into this branch as part of latest commits)

Per business owner clarifications added configurable constant to use as
default zoom url link for city council meetings:
'https://sanjoseca.zoom.us/j/88471470054'
Store default mtg zoom url as envmt var along with styling fixes
@JMStudiosJoe JMStudiosJoe merged commit b746133 into codeforsanjose:develop Nov 16, 2023
0 of 3 checks passed
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