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

PE-D: Fix documentation bugs mentioned in PE dry run #234

Merged
merged 16 commits into from
Nov 1, 2021

Conversation

JeffZincatz
Copy link

@JeffZincatz JeffZincatz commented Oct 31, 2021

Fix various documentation bugs mentioned in PE dry run.

  • Ui.png is now up to date with the new color scheme
  • Standardize all usage of "for example" abbreviations to "e.g., "
  • Fix typo of missing title= in UG example add_p Assistant
  • Fix different date formatting in UG code summary from app (now in the form of e.g., 18 Oct 2021)
  • Fix typo in command summary for the example of delete_p
  • Fix error in command summary for find_c and find_p
  • Reorder command summary to match the order of ToC
  • Fix typo in Assign candidates to an interview section title (unassign to assign)
  • Fix error in add_c that the position field was wrongly indicated as optional

Fix #161 Fix #168 Fix #176 Fix #177 Fix #181 Fix #184 Fix #191 Fix #199 Fix #233

JeffZincatz and others added 8 commits October 31, 2021 16:42
* updated interviewUtil's method to convert an interview to a string in user input format
* added more test cases to existing test classes
* added test classes RemarkCandidateCommandParserTest and AddPositionCommandParserTest
* added TupleTest
* added test cases to various classes
* fixed checkstyle errors
* changed method assertEditSuccess to call the other constructor of commandResult
* removed a test case that causes build failure in build(mac-os)
@JeffZincatz JeffZincatz changed the title PE-D: Fix bugs mentioned in PE dry run PE-D: Fix documentation bugs mentioned in PE dry run Oct 31, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2021

Codecov Report

Merging #234 (05e7286) into master (762db97) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #234   +/-   ##
=========================================
  Coverage     80.29%   80.29%           
  Complexity     1005     1005           
=========================================
  Files           119      119           
  Lines          2989     2989           
  Branches        417      417           
=========================================
  Hits           2400     2400           
  Misses          469      469           
  Partials        120      120           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 762db97...05e7286. Read the comment docs.

@JeffZincatz JeffZincatz marked this pull request as ready for review October 31, 2021 09:53
@JeffZincatz JeffZincatz added this to the v1.4 milestone Oct 31, 2021
| **Delete an interview** | `delete_i <INDEX>` e.g., `delete_i 1` | Deleted Interview: [Accountant [Bernice Yu, David Li] 18 Oct 2021 14:00 - 16:00 PENDING] |
| **Edit an interview** | `edit_i <INDEX> [position=POSITION]... [c=<INDEX>]... [date=DATE]... [time=TIME]... [duration=DURATION]... [interviewed=STATUS]...` e.g., `edit_i 2 c=1 2 date=21/10/2021 time=1400` | Edited Interview: [Data Analyst [Jenny Lim, Max Tan] 21 Oct 2021 14:00 - 16:00 PENDING] |
| **List all interviews** | `list_i` | Listed all interviews <br> 1. [Accountant [Bernice Yu, David Li] 18 Oct 2021 14:00 - 16:00 PENDING] <br> 2. [Project Manager [Bernice Yu] 20 Oct 2021 15:00 - 16:00 PENDING] |
| **Unassign candidates** | `unassign i=<INTERVIEW_INDEX> c=<CANDIDATE_INDEX>...` e.g., `unassign i=1 c=4`| Candidates removed from interview: [Project Manager [Bernice Yu] 20 Oct 2021 15:00 - 16:00 PENDING]: <br> 1. David Li |
Copy link

Choose a reason for hiding this comment

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

I think for issue #177, we discussed that it a feedback error. Because Interview card is in YYYY-MM-DD. Only the feedback is in DD MM YYY

Copy link
Author

Choose a reason for hiding this comment

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

So the correct format in UG should still be YYYY-MM-DD, and the feedback in the application should be changed to be the same? If so, then I will revert the changes.

Copy link

@wanyu-l wanyu-l Oct 31, 2021

Choose a reason for hiding this comment

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

I think might be better to leave as it is (and maybe change interview card accordingly), because otherwise input is DD/MM/YYYY but display is YYYY-MM-DD. On the other hand "18 Oct 2021" is closer to user's input, but of course can change it to "18-10-2021" or something else entirely also

Copy link

Choose a reason for hiding this comment

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

I also think displaying DD MM YYYY for interview card honestly. I feel ppl are more inclined towards DD MM YYYY than YYYY-MM-DD.
So I think can merge, but create an issue to update the interview card to show DD MM YYYY.

Copy link

Choose a reason for hiding this comment

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

Sure, I'll merge it now since it looks good otherwise

Copy link

@wanyu-l wanyu-l left a comment

Choose a reason for hiding this comment

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

LGTM. Will add the issue for Interview Card now.

@wanyu-l wanyu-l merged commit b530b0f into AY2122S1-CS2103T-W13-1:master Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment