-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Modular behaviors + Keyboard support + Move scrollable zone outside #347
base: master
Are you sure you want to change the base?
Conversation
addon/behaviors/behavior.js
Outdated
events: null, | ||
|
||
init() { | ||
this._super.call(this, ...arguments); |
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.
this._super(...arguments)
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.
Please refactor to use ES6 syntax
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.
Will do
addon/behaviors/multi-select.js
Outdated
onSelectRow(ltBody, row) { | ||
let isSelected = row.get('selected'); | ||
if (this.get('requiresKeyboard')) { | ||
this._onRowClick(ltBody, row, function(i, table) { |
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.
() => {}
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.
Please refactor to use ES6 syntax
addon/behaviors/row-expansion.js
Outdated
this.events.onToggleExpandedRow = [ 'rowClick:_all' ]; | ||
}, | ||
|
||
onToggleExpandedRow(ltBody, row) { |
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.
Is there a reason you are passing the entire ltBody component here and not just the table 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.
I don't like it either. It was simply to make mixins/has-behaviors
more generic, i.e. it is simply passing this
as the first parameter. Not a very good reason, I aggree. Maybe we can instruct mixins/has-behaviors
what to pass as the first argument. What do you think ?
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 dont think that a behavior should have interaction with the lt-body component. Everything should be done on the table instance which should be then reflected on the required components in the view layer.
Passing around the component is considered bad practice since it can be destroyed at any time. I would much rather have the table instance be passed around.
Maybe even just init the behavior with the table instance? That way you only have to do it once. Im not exactly sure of the cleanest route for that though.
addon/components/lt-body.js
Outdated
@@ -350,6 +365,25 @@ export default Component.extend({ | |||
} | |||
}, | |||
|
|||
_onFocusedRowChanged: Ember.observer('table.focusIndex', function() { | |||
run.schedule('afterRender', null, () => { | |||
let row = this.$('tr.has-focus'); |
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.
Would it be easier to use the scrollToRow
action 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.
Possibly, I will look into 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.
I am unable to make it work with scrollToRow. I will leave it like this until someone figures this out.
addon/index.js
Outdated
Table, | ||
Column, | ||
Row, | ||
FocusBehavior, |
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 don't think we need to export the behaviors. Who ever needs them can grab them from the behavior dir.
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.
Ok, no problem.
addon/mixins/has-behaviors.js
Outdated
/* | ||
* from ember-keyboard | ||
*/ | ||
const gatherKeys = function gatherKeys(event) { |
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.
Is there a reason we are grabbing this entire file from ember-keyboard?
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.
The gatherKeys
function is not part of their public API but I wanted to use the ember-keyboard syntax for row click event names (rowClick:shift, rowClick:shift+ctrl, etc.), to keep things consistent. So I just copied the method that gather the modifiers keys from the event.
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.
Ah that was my misunderstanding. I thought you took the entire file from ember-keyboard.
@plcarmel this is pretty damn cool. I've been meaning to do something like this for a while but never got around to it. Thank you for taking the lead on this. I've started with just some general comments/ questions. I still need to do a deep review on the rest of the code once I get a bit more time on my hands. I think the API around applying behaviors could be a bit cleaner but we can work on that once we have a fully working solution 😄 |
I think it would be also really handy to support |
0d43d08
to
8f50e50
Compare
Thank you @offirgolan ! Yes, I agree the behavior API could be cleaner. I look forward to suggestions on how to improve this. Having behavior helpers supporting actions is a good idea. |
@plcarmel let me know when you're at a stable place and I'll do a deep dive into this PR 😄 In terms of the API for the behavior helpers. I think you should just have a single helper. So instead of: {{#t.body
behaviors=(array
(lt-row-focus)
(lt-row-expansion multiRow=false)
)
}} You will have something like: {{#t.body
behaviors=(array
(lt-behavior 'row-focus' onFocus=(action 'foo'))
(lt-behavior 'row-expansion' multiRow=false onExpand=(action 'bar'))
)
}} |
@offirgolan, it though about it an I think it is not a bad idea to pass the Here's a use case: And it is true that the component can be destroyed at any time, but we pass the reference to the functions of the behavior instance, not to the behavior instance itself. A behavior should not save this reference for future usage and we can put emphasis on this in the documentation. What do you think ? |
@plcarmel I think your case is valid. I actually think that this sort of concept is pretty interesting and can have a huge benefit to a lot of apps / addons in the ecosystem. The ability to define tasks that will be evented via actions / keyboard events and are centralized to a single modular location is pretty damn nice to have. This is why I have taken your general idea and created a new addon for it. I took it in a bit of a different direction which I think makes the implementation much cleaner and much more modular. I'd love for you to check it out so we can work on it together. You can check out the repo at offirgolan/ember-evented-tasks. |
I share your opinion. That was my plan to eventually make an add-on out of it but I am happy that you are taking it on yourself to start it. My only concert with the concept of evented task is that, by making it too general, we might loose some of the properties of behaviors (behaviors that are mutually exclusive, behaviors having a higher priority, etc.) Maybe one can be built on top of the other ? |
@plcarmel that's the plan. The idea of the evented tasks to to be as general as possible so other apps/addons can build on top of it. I prefer to have this sort of logic as a 3rd party addon so we can thoroughly test it. It seems as though you have come across a good amount of obstacles as you mentioned so maybe we can try to generalize some of those as much as possible and port it over to Thoughts? |
Sounds good to me 😄 |
Sweet! Feel free to open up PRs / issues. |
3ab724e
to
0c3f34b
Compare
@plcarmel I think that ember-evented-tasks is now in a pretty good state. I have added you as a collaborator to the repo so we can work together on the whole prioritization and mutually exclusive stuff. Also, I'm not sure if |
@offirgolan My strategy right now is to end my implementation of spreadsheat-select in this PR before I start to work on / use your addon. This way, I feel that we will end up with something that cover more use cases. |
@plcarmel no rush 😄 |
48eed45
to
899d8a5
Compare
@offirgolan, I hope you are doing well. I updated the demo and the branch of the PR. https://plcarmel.github.io/ember-light-table/#/rows/spreadsheet I implemented almost all the features that I wanted. Selection is now much easier with the mouse. It is even possible to resize existing selections using handles. However, Unfortunately, I now have to take a pause of at least a couple of weeks to work on some urgent stuff. I will let you know when I am ready to give this some more time. |
6946068
to
7f0fb4e
Compare
@plcarmel that's great to hear! Im super excited to see this PR progress 😄 |
Hi @offirgolan, I consider that I am done. It only took 17 more months than expected... Oh well... https://plcarmel.github.io/ember-light-table/#/rows/spreadsheet Everything works in the demo application (except the horizontal scrolling with a virtual scrollbar, which is not currently working anyway). It is almost fully compatible. The only incompatibility is that headers and footers are now inlined by default. This is a consequence of the fact that I removed the scrollable zone from inside the ember-inline-table component. It now requires a bit more wiring to get fixed headers and footers (or a scrollable table for that matter). In my project, I need a scrollable zone located outside the ember-light-table component to be able to use a scrollbar on the body tag and still be able to ask the table to make sure a given row is visible (i.e. ask it to scroll to the requested row). Having a scrollbar on the body tag also makes the experience much better on mobile devices. |
@alexander-alvarez just made me realize there is a 2.0 branch. I looked at it and jQuery got removed, which is good. However, I rely heavily on it so I'll need to refactor my code. Once this is done, do you think it could be merged in 2.0 ? |
Hey @plcarmel . Thanks so much for all of this work, and coming back to it even after such a large amount of time. I definitely want to include what we can into ELT! Would it be possible to break apart your work into more concise changes? This way we can incorporate them incrementally into the main branch.
|
Working on it. |
Ok, I did an effort to break it into pieces. "Behaviors" API and basic behaviors are now independent of the new structure. However, interesting behaviors like "row-focus" and "spreadsheet" interact with scrolling and I really don't have the energy to retrofit them to the old structure. @alexander-alvarez, how do you want to proceed from here ? |
Awesome thank you @plcarmel I'll need to take some time to thoroughly review your work, but I appreciate all the work you've already done to get this here. Expect clarifying questions, comments about documentation and also doing this without jquery. I need to do a better job of highlighting this, but in case you don't know, right now we have a 2.x branch that has some breaking changes in it (including being jquery-less), so we'd introduce those there. For example, we consider major DOM layout / CSS changes breaking changes. To that point ea427aa and 50d3fd1 can likely make it into our master branch, whereas fda286c (and anything dependent on it) may be something that's better to target our 2.x branch. I'll have more for you once I review, but I wanted to give you heads-up in case you wanted to start thinking about these things with me. Thanks again for your hard work! |
What can I do to speed things up, other than removing the dependency on jquery, which I'll try to do asap ? |
7e132e9
to
d3edb0d
Compare
When multiple tables were rendered to the document, only the first table would scroll correctly. This was due to the selector referencing a class name and ember-in-viewport only selecting the first instance of that class. This commit prepends a table ID to that selector so that multiple tables with scrolling is supported.
👋 We just merged the 3-x branch into master. Can you please rebase your PR? |
@plcarmel Thanks for the PR! We've recently made a lot of significant changes to ember-light-table. Can you please rebase your PR and make sure everything is still working as intended? If this PR is no longer necessary after the new changes, please close it. Thanks! |
To resolve #236
Work in progress.
I will...