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

fix: make the verify and captures pages consistent #152

Merged
merged 2 commits into from
Sep 28, 2021
Merged

fix: make the verify and captures pages consistent #152

merged 2 commits into from
Sep 28, 2021

Conversation

tranquanghuy0801
Copy link
Member

Resolves issue #51

nmcharlton
nmcharlton requested changes Sep 17, 2021
View reviewed changes
Copy link
Collaborator

@nmcharlton nmcharlton left a comment

Choose a reason for hiding this comment

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

Excellent work @tranquanghuy0801!
Just a few minor comments.

src/components/CaptureTable.js Outdated Show resolved Hide resolved
src/components/CaptureTable.js Outdated Show resolved Hide resolved
src/components/CaptureTable.js Outdated Show resolved Hide resolved
src/components/CaptureTable.js Outdated Show resolved Hide resolved
src/components/FilterTop.js Outdated Show resolved Hide resolved
@nmcharlton nmcharlton linked an issue Sep 17, 2021 that may be closed by this pull request
Make Verify and Captures pages more consistent #51
Closed
@tranquanghuy0801
Copy link
Member Author

@nmcharlton Thanks for the feedback. I have made the changes as you requested

@tranquanghuy0801
Copy link
Member Author

@nmcharlton , I think the issue #110 relates to this. We already had UUID field in Capture Detail dialog. Do we need to put UUID field in the top filter as well?

nmcharlton
nmcharlton approved these changes Sep 22, 2021
View reviewed changes
Copy link
Collaborator

@nmcharlton nmcharlton left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks @tranquanghuy0801!

@nmcharlton
Copy link
Collaborator

@nmcharlton , I think the issue #110 relates to this. We already had UUID field in Capture Detail dialog. Do we need to put UUID field in the top filter as well?

Let's get this PR merged and aligned with the @gwynndp's refactor, then address #110 under a separate PR.

@tranquanghuy0801
Copy link
Member Author

@nmcharlton , do I have to push more commits to resolve these conflicts?

@nmcharlton
Copy link
Collaborator

@nmcharlton , do I have to push more commits to resolve these conflicts?

It would be great if you're able to resolve the conflicts and push the result.
It's likely to take some time and care to get right - let me or @gwynndp know if you want some help.

@dadiorchen
Copy link
Collaborator

@tranquanghuy0801 you can merge master into your branch and resolve conflicts and push

@tranquanghuy0801
Copy link
Member Author

@tranquanghuy0801 you can merge master into your branch and resolve conflicts and push

yeah, I am doing that.

nmcharlton
nmcharlton requested changes Sep 27, 2021
View reviewed changes
Copy link
Collaborator

@nmcharlton nmcharlton left a comment

Choose a reason for hiding this comment

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

Just a couple of merge issues

src/components/FilterTop.js Outdated
@@ -79,7 +80,6 @@ function Filter(props) {
const filterOptionAll = 'All';
const dateStartDefault = null;
const dateEndDefault = null;
const [uuid, setUUID] = useState(filter?.uuid || '');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the UUID change has been accidentally removed in the merge.

src/components/FilterTop.js Outdated
@@ -161,24 +159,6 @@ function Filter(props) {
props.onSubmit && props.onSubmit(filter);
}

const defaultOrgList = userHasOrg
Copy link
Collaborator

Choose a reason for hiding this comment

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

This block needs to stay

src/components/FilterTop.js Outdated
@@ -387,8 +359,7 @@ function Filter(props) {
// clearOnBlur
// handleHomeEndKeys
/>
{
/* {!userHasOrg && ( }*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the org changes here need to stay in

nmcharlton
nmcharlton approved these changes Sep 28, 2021
View reviewed changes
Copy link
Collaborator

@nmcharlton nmcharlton left a comment

Choose a reason for hiding this comment

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

Great work 😎

@nmcharlton nmcharlton merged commit c8f5cd3 into Greenstand:master Sep 28, 2021
@tranquanghuy0801 tranquanghuy0801 deleted the fix-51 branch September 30, 2021 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nmcharlton nmcharlton nmcharlton approved these changes

@gwynndp gwynndp Awaiting requested review from gwynndp

@luacmartins luacmartins Awaiting requested review from luacmartins

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make Verify and Captures pages more consistent
3 participants
@tranquanghuy0801 @nmcharlton @dadiorchen