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.

Sign up for GitHub

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

Jump to bottom

feat: campaign form #9

Merged
merged 6 commits into from
Jul 10, 2024
Merged

feat: campaign form #9

merged 6 commits into from
Jul 10, 2024

Conversation

richardhjtan
Copy link
Collaborator

Refer to https://docs.google.com/document/d/17aH9dPGSLU73AugLtOIK_siLOYfVc0YgFPEojJlxsyw/edit#heading=h.s6x2suuuqma

What is changing

  • add new campaign form as a card
  • different view on campaign form: Isolated / Atom / Embedded / Edit
  • handling number input with formatting while edit
  • handling start date / end date interaction while edit

@richardhjtan richardhjtan requested a review from tintinthong June 18, 2024 05:13
@richardhjtan richardhjtan self-assigned this Jun 18, 2024
@tintinthong
Copy link
Owner

tintinthong commented Jun 19, 2024 •
edited
Loading

Good first pass. Is it possible to do step-by-step. Let's first create a percentage field. I can pair with u on this (prefererably in a separate PR). Next create a currency field.

@tintinthong
Copy link
Owner

Install glint extension, turn off typescript @Builtin and pnpm lint:fix

@tintinthong
Copy link
Owner

tintinthong commented Jun 21, 2024 •
edited
Loading

To recap our pairing discussion

  • we shud make percentage work like number when inputting field. But must help user understand it is a percentage
  • we shud display % and , in isolated
  • dont need to write new PR about percentage
  • still same opinion, try to use get rather than tracked (where tracked is not needed)
  • i merged main branch into crm-use-case

@tintinthong tintinthong self-requested a review June 28, 2024 08:14
tintinthong
tintinthong approved these changes Jun 28, 2024
View reviewed changes
Copy link
Owner

@tintinthong tintinthong left a comment

Choose a reason for hiding this comment

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

LGTM. I added some small tweak comments. Pls fix

@tintinthong tintinthong merged commit 1fee607 into tintinthong:crm-use-case Jul 10, 2024
20 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tintinthong tintinthong tintinthong approved these changes

Assignees

@richardhjtan richardhjtan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@richardhjtan @tintinthong