-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
[IMPROVE] UI design for Tables and tabs component on Directory #11026
Conversation
Why is the name of this pull request labeled as
|
@graywolf336 ... the design is new, but we didnt change nothing as feature, just fixed the bad UI, or am I wrong? |
Should we instead use an improve tag? Instead of fix or new. |
@geekgonecrazy Good call, an IMPROVE tag would make the most sense here. Furthermore, by using a tag like that, we might get more people to polish up the existing parts of RC, something quite important to good UX and code :-) |
I agree @geekgonecrazy , but I thought we had just NEW, BREAK, FIX |
@ggazzo that's the issue, I think~ |
@ggazzo right we only have the 3. I'm suggesting we consider one more. That way things such as component redesign can be surfaced separately so we can make it more clear the components we improve vs the ones we fix a bug only. Like some times we completely rework something for the purpose of improving a lot of things and it does close bugs, but over all we improved functionality. Just an idea. @rodrigok what do you think? That or should we add a tag to PR's that it's design, that way they can be found that way? Could also surface based on that in changelog? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
.tab { | ||
margin: 0 1rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should use transition: color .3s;
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Template.table.events({ | ||
'click tbody tr'() { | ||
const onItemClick = Template.instance().data.onItemClick; | ||
if (onItemClick) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return onItemClick && onItemClick(this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
Template.tabs.events({ | ||
'click .tab'(e) { | ||
$('.tab').removeClass('active'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this way will not work for nested tabs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how nested tabs should work? We will have nested tabs? We already have a design for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we dont have design to nested to show you now, but just to think...
, and we can have more than one per page... you are looking for ALL .tabs
and maybe you are creating a bug
you did here, why not keep inside the component?
Template.tabs.onRendered(function() {
this.$('.tab').first().addClass('active');
this.data.tabs.onChange(this.data.tabs.tabs[0].value);
});
and more we are using a reactive framework, please change that using reactive vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do a simple {activeClass}
variable on the helper and change according to state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a 'ReactiveVar' to keep the state of the active tab and adde a helper to check which tab is active and add an active class to this tab.
Thank you for your suggestions :)
}; | ||
}, | ||
onTableItemClick() { | ||
const searchType = Template.instance().searchType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { searchType } = Template.instance();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@kaiiiiiiiii Thank you for your report :) I already fixed the first problem. @ggazzo ☝️ |
Hey @kaiiiiiiiii we will remove channel description from this table layout for now. In a near future we will add a extended (with descriptions) and compact layout for this table, just like we did with the sidebar. Thank again for your reports :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
A better design for tables and tab component, applied on Directory page.
Still needs to write a documentation about these new components and how the directory feature works. We can make a blogpost. What you @RocketChat/core think?