search

NoroffFEU / yeetsheet

1 stars 4 forks source link

Review and merge PR #73 #91

Closed sulenchy closed 1 month ago

sulenchy commented 1 month ago

Discuss this ticket with your team. Although it is a QA ticket, the QA engineer may still request some changes, so it is necessary to assign each PR to a developer. The developer will handle the requested changes.

PR #73

FredrikMohag commented 1 month ago

Console output are added but after displaying the logs in the code-console-output, it might be good to clear the console.logs, console.warns, and console.errors arrays to avoid showing the same logs multiple times and to keep memory usage low.

In the showTab function, make sure to define event as a parameter or pass this when calling the function. This will prevent issues when event.currentTarget is used. For example, update the function to accept event as an argument, or modify the button call like this: onclick="showTab('console-tab', this)

@Berrinj

Barvand commented 1 month ago

I spotted an error here.

The title says PR #73 however in the message from Abi it tagged #72. I have updated this to PR #73

So please refer to PR #73 as the other team are working on 72. @FredrikMohag @Berrinj

I am sorry @FredrikMohag you accidentally checked the wrong PR. Could you please review PR #73 and let @Berrinj know if there are any updates required.

Berrinj commented 1 month ago

I was very much confused before you wrote that. I haven't started the coding just getting familiar so no worries

FredrikMohag commented 1 month ago

Thanks for sorting that out @Barvand Should this PR move to "In progress"? @Berrinj

Barvand commented 1 month ago

Hi @FredrikMohag

Could you check if PR#73 needs any improvements? If it does need some improvements please but it back to in progress. If the code is good and quality checked you can merge and put it into the done section!

Bart

Berrinj commented 1 month ago

Yes👍🏻

FredrikMohag commented 1 month ago

Ok found some improvements. @Berrinj

Error Handling: Add a check to log an error if any elements (toggleButton, sidebarContent, etc.) are missing to make debugging easier.

Accessibility: Consider adding the aria-expanded attribute to the toggleButton to improve accessibility for screen readers.

These updates will enhance the robustness and user experience 👍

Berrinj commented 1 month ago

@FredrikMohag closing ticket, togglefunction is outdated based on the newest design/changes

Barvand commented 1 month ago

This has been merged with the workflow branch. Function is not working anymore due to the website being updated. E.g. the aside has been removed. Too many changes in a short time have deemed this function outdated. Will implement a fix.