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

Feat/table view for issues #1648

Merged
merged 268 commits into from
Oct 26, 2023
Merged

Feat/table view for issues #1648

merged 268 commits into from
Oct 26, 2023

Conversation

YehualashetGit
Copy link
Contributor

@YehualashetGit YehualashetGit commented Oct 25, 2023

* create data table ui component
* create re usable table component and used them for Data table UI component
* refactor team members componen so that it can show card and table component
* add a swtich button to change issue view

https://www.veed.io/view/f1ebbac3-64a1-4423-8e96-05685b3ea2b6?panel=share

@badalkhatri0924
Copy link
Contributor

@YehualashetGit why needed to force push?

It doesn't allow me to push after merging the conflict

@evereq what do you suggest here?

@badalkhatri0924
Copy link
Contributor

image Also @YehualashetGit it is not loading data for me.

@YehualashetGit
Copy link
Contributor Author

YehualashetGit commented Oct 25, 2023

image Also @YehualashetGit it is not loading data for me.
let's wait until I finalize it ,it's still under Draft PR, I will make it ready for return teamMembersView;
once I'm done, here I was doing the filter by user id ,

@evereq
Copy link
Member

evereq commented Oct 25, 2023

@YehualashetGit / @badalkhatri0924 I think issue was probably that feature branch was created from master branch instead of develop. But it's not a big problem, we will just merge this PR using "squash" when it's ready so it will be single commit to develop etc. So let's keep working on it :)

@@ -116,3 +116,9 @@ export const APPLICATION_LANGUAGES_CODE = [
'ru',
'es'
];

export enum KanbanView {
Copy link
Member

@evereq evereq Oct 26, 2023

Choose a reason for hiding this comment

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

@YehualashetGit Maybe let's rename this to "IssuesView"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure I will rename that

Copy link
Member

Choose a reason for hiding this comment

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

Btw, I think values should be:
CARDS, TABLE, and BLOCKS, please adjust @YehualashetGit :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@@ -225,9 +225,9 @@ export function InviteUserTeamCard({
<DraggerIcon className="fill-[#CCCCCC] dark:fill-[#4F5662]" />
</div>

<div className="absolute opacity-40 right-2">
{/* <div className="absolute opacity-40 right-2">
Copy link
Member

Choose a reason for hiding this comment

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

@YehualashetGit why this got commented out? Not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes we don't need this code s actually, basically the invite row won't have such menu , may be i will remove it

Copy link
Member

Choose a reason for hiding this comment

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

Yes, makes sense @YehualashetGit

Copy link
Member

Choose a reason for hiding this comment

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

@YehualashetGit On the other hand, idea was to make that Invite row to look similar to other rows... Iike it's still "empty" but when you invite someone that person will be shown there with task and other info... So I would prefer you to restore it back :)))

Also in the future when you removing something (even if it looks like not used), please state it in the PR explicitly - we should be very careful with remove things if we not sure about initial idea why it was added etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay understood , the main reason why I removed it was , there was no real data to display and they are just a place holder , that's why , but will reverted it back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evereq is this a temporary or it only be visible on Table view cause ,it'll now make me to add more logic on TeamMeberTable View component to the invite row with separate actions

@evereq
Copy link
Member

evereq commented Oct 26, 2023

@YehualashetGit see my comments and let's fix this quickly so we can merge as soon as possible, thanks!
Also, most probably we will have to make more improvements to the table view later. Honestly the idea of table view for "Issues" was different - it should be that "Issue title/description" would go as the first column, etc, but what you essentially did is exactly the same view we had for "horizontal cards" but now implemented also as a table (more condensed) view and I LIKE that.

So we will merge this and you will work to make 3rd view for the same data (block view), but essentially we will need to also work on a real "issues" page later. Let's discuss that later in Slack, for now let's finish this PR merge it, and create a "block" view next (from our Figma):

image

@badalkhatri0924
Copy link
Contributor

image

@YehualashetGit there are some UI issues

@badalkhatri0924
Copy link
Contributor

image

@YehualashetGit UI issue.

@evereq
Copy link
Member

evereq commented Oct 26, 2023

@YehualashetGit you working on fixes for those issues right?

@YehualashetGit
Copy link
Contributor Author

@YehualashetGit you working on fixes for those issues right?

yes I've fixed the ui issues

@evereq
Copy link
Member

evereq commented Oct 26, 2023

@badalkhatri0924 can you review again please? If something still broken try to quick fix yourself and let's merge this one so I can review it online

@evereq evereq merged commit 53d03c5 into develop Oct 26, 2023
3 of 4 checks passed
@evereq evereq deleted the feat/table-view-for-issues branch October 26, 2023 17:26
@evereq
Copy link
Member

evereq commented Oct 26, 2023

@YehualashetGit I merged with "squash", so please don't use anymore that same branch, create a new one based on develop for all future work on those tasks. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants