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

CS169L SP24: Iter 2-4 Changes #314

Merged
merged 108 commits into from
May 11, 2024

Conversation

realmichaeltao
Copy link
Contributor

@realmichaeltao realmichaeltao commented Apr 26, 2024

This PR incorporates many changes

  • tooltip bug fix on admin tables
  • multiple email addresses per teach
  • languages for teachers
  • PD model
  • pd registrations

Pivotal Tracker Link

What this PR does:

This pull request addresses a bug related to tooltip persistence upon clicking checkboxes. Previously, tooltips remained visible even after clicking a checkbox. The solution involves triggering a redraw of the table with $tables.draw() upon a checkbox change. Additionally, during each table redraw, all tooltips are first disposed of and then reinitialized. This ensures that tooltips only appear when needed and do not persist inappropriately.

Include screenshots, videos, etc.

Before:
Screenshot 2024-04-25 at 17 23 50
After:
Screenshot 2024-04-25 at 17 23 55

Who authored this PR?

@perryzjc
@realmichaeltao

How should this PR be tested?

  • Is there a deploy we can view?
    Yes
  • What do the specs/features test?
    Since this change primarily affects frontend functionality, traditional backend testing frameworks like Cucumber or RSpec are not applicable here.
  • Are there edge cases to watch out for?
    Check tooltip behavior when multiple checkboxes are interacted with in quick succession.

Are there any complications to deploying this?

No

Checklist:

  • Has this been deployed to a staging environment or reviewed by a customer?
  • Tag someone for code review (either a coach / team member)
  • I have renamed the branch to match PivotTracker's suggested one (necessary for BlueJay) (e.g. michael/12345-add-new-feature Any branch name will do as long as the story ID is there. You can use git checkout -b [new-branch-name])

ArushC and others added 30 commits March 14, 2024 09:54
Align frontend for PD CRUD to be similar to the schools page
@cycomachead cycomachead self-requested a review May 10, 2024 09:32
@cycomachead cycomachead changed the title Tooltip bug fix CS169L SP24: Iter 2-4 Changes May 10, 2024
Copy link
Member

@cycomachead cycomachead left a comment

Choose a reason for hiding this comment

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

Lots of good stuff here, some minor fixes needed.

app/views/teachers/_form.html.erb Outdated Show resolved Hide resolved
Comment on lines +70 to +75
<%= teacher.primary_email %>
<% if teacher.personal_emails.present? %>
<% teacher.personal_emails.each do |email| %>
<br><%= email %>
<% end %>
<% end %>
Copy link
Member

Choose a reason for hiding this comment

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

Fine for now, but maybe this should be a helper which returns the right html

Comment on lines +109 to +110
<div class='form-group row'>
<div class='col-12'>
Copy link
Member

Choose a reason for hiding this comment

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

Fix this in the future

app/views/teachers/_form.html.erb Outdated Show resolved Hide resolved
@@ -19,6 +20,9 @@
resources :schools
resources :pages, param: :url_slug
resources :email_templates, except: [:show]
resources :professional_developments do
resources :pd_registrations, except: [:show]
Copy link
Member

Choose a reason for hiding this comment

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

TODO: Verify the set needed. I don't think we need #index or #new, #edit routes. Do we even need #update or just create/delete

@@ -163,6 +168,24 @@ def self.education_level_options
Teacher.education_levels.map { |sym, val| [sym.to_s.titlecase, val] }
end

def self.language_options
Copy link
Member

Choose a reason for hiding this comment

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

TODO: this is not really necessary.

validates :email, uniqueness: true
validates :personal_email, uniqueness: true, if: -> { personal_email.present? }
validate :ensure_unique_personal_email, if: -> { email_changed? || personal_email_changed? }
WORLD_LANGUAGES = [ "Afrikaans", "Albanian", "Arabic", "Armenian", "Basque", "Bengali", "Bulgarian", "Catalan", "Cambodian", "Chinese (Mandarin)", "Croatian", "Czech", "Danish", "Dutch", "English", "Estonian", "Fiji", "Finnish", "French", "Georgian", "German", "Greek", "Gujarati", "Hebrew", "Hindi", "Hungarian", "Icelandic", "Indonesian", "Irish", "Italian", "Japanese", "Javanese", "Korean", "Latin", "Latvian", "Lithuanian", "Macedonian", "Malay", "Malayalam", "Maltese", "Maori", "Marathi", "Mongolian", "Nepali", "Norwegian", "Persian", "Polish", "Portuguese", "Punjabi", "Quechua", "Romanian", "Russian", "Samoan", "Serbian", "Slovak", "Slovenian", "Spanish", "Swahili", "Swedish ", "Tamil", "Tatar", "Telugu", "Thai", "Tibetan", "Tonga", "Turkish", "Ukrainian", "Urdu", "Uzbek", "Vietnamese", "Welsh", "Xhosa" ].freeze
Copy link
Member

Choose a reason for hiding this comment

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

Feels like we can put this somewhere else.

app/models/professional_development.rb Outdated Show resolved Hide resolved
app/models/professional_development.rb Outdated Show resolved Hide resolved
app/models/pd_registration.rb Outdated Show resolved Hide resolved
@cycomachead
Copy link
Member

cycomachead commented May 10, 2024

  • fixup cucumber
  • fix PdRegistrations to be PDRegistrations?
  • emails, PDs, cleanup up controller methods to have the minimum actions
  • We should move state/country validations to a concern shared with schools/pds

@cycomachead cycomachead merged commit e0df660 into beautyjoy:main May 11, 2024
8 checks passed
@cycomachead cycomachead deleted the 187460464/bugfix/tooltip branch May 11, 2024 00:42
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.

5 participants