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

BC-8578 - Remove Visibility Alert From Board #3516

Merged
merged 14 commits into from
Jan 27, 2025

Conversation

odalys-dataport
Copy link
Contributor

@odalys-dataport odalys-dataport commented Jan 22, 2025

Short Description

Removes alert from board about its visibility and adds a tooltip to the "draft-chip".

Links to Ticket and related Pull-Requests

BC-8578

Changes

Data-security

Deployment

New Repos, NPM packages or vendor scripts

Screenshots of UI changes

Screenshot 2025-01-22 at 14 21 19

Checklist before merging

  • QA: In addition to review, the code has been manually tested (if manual testing is possible)
  • PO: Any deviation from requirements was agreed with Product-Owner / ticket author / support-team
  • DEV: Every new component is implemented having accessibility in mind (e.g. aria-label, role property)

Notice: Please keep this Pull-Request as a Draft (or add WIP label), until it is ready to be reviewed

@odalys-dataport odalys-dataport self-assigned this Jan 22, 2025
@odalys-dataport odalys-dataport marked this pull request as ready for review January 22, 2025 14:52
@@ -328,25 +326,9 @@ onUnmounted(() => {
resetNotifierModule();
});

const setAlert = useDebounceFn(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove import of useDebounceFn above

src/modules/feature/board/board/BoardHeader.vue Outdated Show resolved Hide resolved
Copy link
Contributor

@MartinSchuhmacher MartinSchuhmacher left a comment

Choose a reason for hiding this comment

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

} from "@@/tests/test-utils/setup";
import BoardDraftChip from "./BoardDraftChip.vue";

describe("@feature-board/BoardDraftChip", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think about two additional tests for text in VChip and VTooltip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imo this falls under the category "testing the framework" as there is no logic or changing texts in this component.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, maybe it would make sense to check if the correct text is rendered inside the component. Something like:

it("should render the text message", () => {
	const wrapper = setup();

	expect(wrapper.text()).toContain("common.words.draft");
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do. Not sure how easy it is to test for the tooltip text though

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. I confused the two components, sorry.

@odalys-dataport odalys-dataport enabled auto-merge (squash) January 27, 2025 15:59
@odalys-dataport odalys-dataport merged commit e2ee88c into main Jan 27, 2025
70 checks passed
@odalys-dataport odalys-dataport deleted the BC-8578-visibility-alert branch January 27, 2025 16:10
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.

3 participants