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

Thread notes #104

Merged
merged 5 commits into from
Oct 16, 2023
Merged

Thread notes #104

merged 5 commits into from
Oct 16, 2023

Conversation

stage-rl
Copy link
Collaborator

@stage-rl stage-rl commented Oct 13, 2023

Otazky, resp. dilemy

  • bude treba este raz prekreslit, a ztailwindui-ovat
  • je to spravene ako dropdown, t.j. cela "zbalena cast" funguje ako dropdown tlacitko, t.j. vieme aj zbalovat
  • uz som trochu zmagoreny z toho, ako velmi nerobit veci v komponente. Snazim sa byt konzistentny s @jsuchal ovymi minulymi komentami. Podla mna uz zbytocne posielam do MessageThreadComponent-u message_thread_note, kedze je takmer vlasnostou threadu
  • Zavedenie pola last_updated_at - usudil som, ze nechcem pouzivat railsovy updated_at, kedze ten sa moze zmenit aj pri pripadnych technickejsich updatoch
  • celkovo je toto pole zbytocne prominentne na zbalenej poznamke
  • este to bude treba zaindexovat do searchu, to poprosim @mirrec , ked budeme mat koncept OK
    zbalena_poznamka
    rozbalena_poznamka
    ulozena_poznamka

@stage-rl stage-rl requested a review from jsuchal October 13, 2023 08:00
Copy link
Member

@jsuchal jsuchal left a comment

Choose a reason for hiding this comment

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

K UX - suhlasim, ze to je prilis prominentne miesto ak to je zbalene. A ak tam nieco je, tak by som ocakaval, ze to nebude zbalene ale bude to vzdy svietit.

Cize pridanie poznamky treba schovat za nejaky maly button a by default nezobrazovat nic.

@message_thread = message_thread
@thread_tags_with_deletable_flag = thread_tags_with_deletable_flag
@thread_messages = thread_messages
@message_thread_note = message_thread_note
Copy link
Member

Choose a reason for hiding this comment

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

toto vies vytiahnut z @message_thread cize netreba explicitne posielat. sice to odpali select ale N+1 query problem nehrozi takze nevidim problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, cize ked nejde cez to iterovat, alebo nepouzivame ako kolekciu, tak si to neposielame. To je asi dobre pravidlo.

Comment on lines 8 to 10
return unless @message_thread_note.last_updated_at

@formatted_last_updated_at = l @message_thread_note.last_updated_at, format: :long
Copy link
Member

Choose a reason for hiding this comment

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

Toto je best practice? nemali by taketo formatovacie veci byt vo view?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Neviem, ci je, resp. asi nie je. Zdalo sa mi to ako prilis komplikovany vyraz na to, aby som ho nechal vo viewe, najprv bol totiz tam. Este mozme pouzit jednu z tych verzii helpera, ktoru diskutujeme v inom vlakne. Si asi vyber.

Copy link
Member

Choose a reason for hiding this comment

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

No praveze taketo sa bezne do view dava


thread.reload
thread.destroy!
thread.merge_identifiers.update_all(message_thread_id: target_thread.id)
Copy link
Member

Choose a reason for hiding this comment

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

zase kuzli linter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, este par krat mi to spravi, a vypnem ho. Zatial mam ale stale rad tie jeho obcasne navrhy na zlepsenia, co prinasa

@@ -0,0 +1,15 @@
# == Schema Information
#
# Table name: message_threads
Copy link
Member

Choose a reason for hiding this comment

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

?

create_table :message_thread_notes do |t|
t.references :message_thread, null: false, foreign_key: true
t.text :note
t.datetime :last_updated_at, null: false
Copy link
Member

Choose a reason for hiding this comment

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

pokial nemame usecase na specialny last_updated_at tak pouzivajme ten defaultny.

@stage-rl
Copy link
Collaborator Author

Ten UX necham ale chalanom doladit. Zatial som spravil aspon to, aby to bolo defaultne rozbalene, ked poznamka existuje. @jsuchal

@stage-rl stage-rl requested a review from jsuchal October 13, 2023 13:33
return unless message_thread_note&.note

if target_thread.message_thread_note
join_string = "\nPripojené z vlákna #{title} #{I18n.l DateTime.now, format: :long}:\n"
Copy link
Member

Choose a reason for hiding this comment

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

Toto takto nie, rozbije sa to na prekladoch. Ja by som tam dal len nejaky separator --- a hotovo.

@stage-rl stage-rl merged commit e7c5da7 into main Oct 16, 2023
3 checks passed
@stage-rl stage-rl deleted the GO-270/thread_notes branch October 16, 2023 11:58
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