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

Minimum and Exact AT Version #1055

Merged
merged 12 commits into from
May 2, 2024
Merged

Minimum and Exact AT Version #1055

merged 12 commits into from
May 2, 2024

Conversation

alflennik
Copy link
Contributor

@alflennik alflennik commented Apr 22, 2024

Adds a new "minimum" and "exact" AT Version picker to the "Add to test queue" UI. This is part of #791.

Note that the test queue status dialog has not yet been updated and is not expected to work.

This PR resolves also conflicts with the development branch that currently prevent merging can-duplicate-test-plan-reports. Therefore this diff includes all the changes from that branch. Sorry for the noise. If we use can-duplicate-test-plan-reports as a base the number of files changed would actually increase.

@alflennik alflennik changed the base branch from development to can-duplicate-test-plan-reports April 23, 2024 15:46
@alflennik alflennik changed the base branch from can-duplicate-test-plan-reports to development April 23, 2024 15:46
@alflennik alflennik requested a review from howard-e April 23, 2024 15:48
Copy link
Contributor

@howard-e howard-e left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Having reviewed a bit of this work already in #997, can confirm the new pieces work as expected. Exciting!

I've left a small suggestion in thread

client/components/AddTestToQueueWithConfirmation/index.jsx Outdated Show resolved Hide resolved
@@ -23,74 +23,6 @@ import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
import DisclosureComponentUnstyled from '../common/DisclosureComponent';

const DisclosureContainer = styled.div`
// Following directives are related to the ManageTestQueue component
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. This should be its own component too probably. (future thought)

);
};

const TextThatWontShiftWhenBold = ({ isBold, children: text }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Descriptive!

browserId: ID!
copyResultsFromTestPlanReportId: ID
copyResultsFromTestPlanVersionId: ID
Copy link
Contributor

Choose a reason for hiding this comment

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

Another good catch

@howard-e
Copy link
Contributor

To reviewers: In a separate discussion with @alflennik we agreed this now introduces breaking changes around the Test Plan Report status dialog. I've added a do not merge label to reflect that. So until additional parts of #791's and #792's implementation requests are ready, this PR should not yet be merged. This will be treated as a current "base" for those issues.

Currently #1001 is also expected to branch off of this work.

@alflennik alflennik changed the base branch from development to trends May 2, 2024 20:26
@alflennik alflennik merged commit 58fb718 into trends May 2, 2024
2 checks passed
@alflennik alflennik deleted the minimum-exact-at-version branch May 2, 2024 20:50
@howard-e howard-e mentioned this pull request Jun 20, 2024
howard-e added a commit that referenced this pull request Jun 20, 2024
This includes work to support #791 and #792.

Includes the following changes:
* #1055
* #1001
* #1065
* #1052 
* #1087
* #1098 
* #1092
* #1131
* #1124

---------

Co-authored-by: Howard Edwards <[email protected]>
Co-authored-by: Paul Clue <[email protected]>
Co-authored-by: alflennik <[email protected]>
howard-e added a commit that referenced this pull request Jun 24, 2024
Includes the following changes:
* #1123, addresses #791 and #792 with:
  * #1055
  * #1001
  * #1065
  * #1052 
  * #1087
  * #1098 
  * #1092
  * #1131
  * #1124
* #1128, addresses #1100
* #1102, addresses #957
* #1132
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants