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

Swik 1827 loading indicator for info panel #754

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

vkovtun
Copy link
Contributor

@vkovtun vkovtun commented Feb 9, 2018

No description provided.

@kadevgraaf
Copy link
Member

kadevgraaf commented Feb 9, 2018

Thanks for the performance improvements!
Do you also want to add a loading indicator similar to, e.g., slide view panel in components/Deck/ContentPanel/SlideModes/SlideViewPanel/SlideContentView.js
Using, e.g.,
{(this.loading === 'loading') ? <div className="ui active dimmer"><div className="ui text loader">Loading</div></div> : ''}
and

    componentWillReceiveProps(nextProps){
[..]
        if (nextProps.loadingIndicator !== this.props.loadingIndicator)
        {
            if (nextProps.loadingIndicator === 'true')
                this.loading = 'loading';
            if(nextProps.loadingIndicator === 'false')
                this.loading = '';
        }
}

W.r.t. the addition of the equals function in common.js -> is this absolutely necessary? I'd rather not add code to the platform if it can be done with native/existing functions.

@vkovtun
Copy link
Contributor Author

vkovtun commented Feb 9, 2018

@kadevgraaf I tried to do that, but it turned out to be not as easy as in the case of the slide view. I will try your suggestion.

@vkovtun
Copy link
Contributor Author

vkovtun commented Feb 22, 2018

@kadevgraaf I forgot to answer your last question. I had to use that function to improve performance. The application has to have an equals method for objects to be able to see whether there have been any changes or not. To the best of my knowledge there is no native function to accomplish that. Another alternative would be JSON.stringify(obj1) === JSON.stringify(obj2), but it suffers from false negatives, since the following objects would not be equal: obj1 = {a: 1, b: 2} & obj2 = {b: 2, a: 1}.

@vkovtun
Copy link
Contributor Author

vkovtun commented Feb 23, 2018

@kadevgraaf I have implemented the loading indicator.

@kadevgraaf
Copy link
Member

Thanks! Works nicely.
1 minor thing - When I view a slide, e.g., http://localhost:3000/deck/4047-1/slide/335-3/335-3:12/view , and I refresh the page using ctrl-R then the first time the info panel keeps loading. The second ctrl-R refresh it does work however. If this is too troublesome to fix I think we can still merge.

Copy link
Member

@kadevgraaf kadevgraaf left a comment

Choose a reason for hiding this comment

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

If I go to slide edit mode, and then save/cancel (go back to slide view mode, the loading indicator keeps loading. Can you fix this @vkovtun ?

Viktor Kovtun added 2 commits September 20, 2018 10:59
…ndicator-for-info-panel

# Conflicts:
#	.gitignore
#	actions/loadContributors.js
#	common.js
#	components/Deck/InfoPanel/InfoPanelInfoView.js
#	stores/ActivityFeedStore.js
…info-panel' into swik-1827-loading-indicator-for-info-panel
@vkovtun
Copy link
Contributor Author

vkovtun commented Sep 20, 2018

@kadevgraaf I cannot reproduce your problem. Can you please check it again? If you can reproduce it, maybe you can send me a link to the slide, where this happens?

Viktor Kovtun added 2 commits November 30, 2018 14:06
# Conflicts:
#	components/Deck/ActivityFeedPanel/ActivityFeedPanel.js
#	components/Deck/InfoPanel/InfoPanel.js
#	components/Deck/InfoPanel/InfoPanelInfoView.js
…s and I merged that deletion with no consideration of the code.
@vkovtun
Copy link
Contributor Author

vkovtun commented Nov 30, 2018

@kadevgraaf I merged master into this branch, fixed the conflicts and merging bugs. Yet I cannot reproduce the issue you have reported.

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.

4 participants