Skip to content

Conversation

@Ishrat420
Copy link

@Ishrat420 Ishrat420 commented Apr 11, 2022

Description

Migrating from Coffeescript to Typescript for web.

#Components

Complete Migration done for the following components:
src/app/errors/not-found
src/app/errors/timeout
src/app/errors/unauthorised
src/app/common/alert-list

##Dependencies and Changes to other files

For migration purposes, had to delete old coffee files:

    deleted:    src/app/common/alert-list/alert-list.coffee
    deleted:    src/app/errors/states/not-found/not-found.coffee
    deleted:    src/app/errors/states/timeout/timeout.tpl.coffee
    deleted:    src/app/errors/states/unauthorised/unauthorised.coffee

Consequently, made changes to the corresponding states file and module file of the component involved in accordance to the MIGRATION-GUIDE.

How Has This Been Tested?

Manually tested each components, works same as before without any performance or technical issue.

Testing Checklist:

  • Tested in latest Chrome

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings

@Ishrat420 Ishrat420 requested review from PerryRose and tancnle April 11, 2022 10:21
@PerryRose
Copy link

PerryRose commented Apr 12, 2022

Hi @tancnle, correct me if I'm wrong, but our branching guidelines outlines one branch per task/sub-task, correct? Additionally, I believe the component migration tasks are required to follow the QA processes and include unit testing before being merged. Let me know if this is correct, Tan. Thanks!

@Ishrat420 Ishrat420 closed this Apr 12, 2022
@tancnle
Copy link

tancnle commented Apr 12, 2022

Thank you for your hard work @Ishrat420 ❤️. You have made some really good progress so far.
Thank you for your feedback on the PR @PerryRose ❤️ .

Hi @tancnle, correct me if I'm wrong, but our branching guidelines outlines one branch per task/sub-task, correct? Additionally, I believe the component migration tasks are required to follow the QA processes and include unit testing before being merged. Let me know if this is correct, Tan. Thanks!

That's a great call-out @PerryRose. I agree. That is the general suggestion. As good practice (good is subjective to teams), we want the smallest change possible, accompanied by tests. It helps to reduce cognitive load for reviewers, speed up code review and reduce surprising bugs.

@Ishrat420 Please let me know if you need help splitting the changes to multiple PRs and adding tests for them. Or perhaps, if you have capacity @PerryRose, can you help @Ishrat420?

Regarding the guidance around Branching and Unit Testing, please help us to improve the doc if things are not clear 🙏🏼

@Ishrat420
Copy link
Author

Ishrat420 commented Apr 12, 2022

@tancnle @PerryRose Thank you so much for reviewing my Pr and for your suggestions. Yes it would be great if someone can assist me in regards to splitting the changes in multiple Pr. And if there is a Documentation on how to approach Unit testing, I can definitely work it out. The linked Documentation is in regards to TDD and I believe it is only applicable when a new feature is introduced, and even then it does not elaborate as in what testing tools to use and how to include them, Unit test usually validates a small portion of the source code, is there any real life examples I can follow? TA

@PerryRose
Copy link

No problems @Ishrat420. I've almost reached the part in my component migration where I will begin looking at writing unit tests. As the team isn't quite familiar with this yet (myself included) it is a good opportunity for us to learn together and provide each other with assistance. I will definitely reach out and provide any help once I starting testing. In regard to splitting a branch - I'm not too familiar with this either. I'd imagine creating 4 separate branches, and referring to the 'File changed' tab in this PR to see your new component code would be an easy way to go about.

@Ishrat420 Ishrat420 deleted the migrate/notfound-timeout-unauthorised-alertlist branch April 15, 2022 05:40
b0ink pushed a commit to b0ink/doubtfire-web that referenced this pull request Jun 23, 2025
fix(feedback-template-editor): improve feedback template sorting logic
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.

4 participants