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

Implement NewsSideBar component #12736

Merged
merged 52 commits into from
May 19, 2021
Merged

Implement NewsSideBar component #12736

merged 52 commits into from
May 19, 2021

Conversation

EVAST9919
Copy link
Contributor

@EVAST9919 EVAST9919 commented May 10, 2021

Basic implementation with no integration. Integration will come in a separate pr.
Implemented API components have been tested, results on the screenshots:
web:
pr_web
pr:
pr_osu

osu.Game/Overlays/News/Sidebar/MonthPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/MonthPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/MonthPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/MonthPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/MonthPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/NewsSideBar.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/NewsSideBar.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/NewsSideBar.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/YearsPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/MonthPanel.cs Outdated Show resolved Hide resolved
Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

Seems fine otherwise.

osu.Game.Tests/Visual/Online/TestSceneNewsMonthPanel.cs Outdated Show resolved Hide resolved
osu.Game.Tests/Visual/Online/TestSceneNewsYearsPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/MonthPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/MonthPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/MonthPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/YearsPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/MonthPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/MonthPanel.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/NewsSideBar.cs Outdated Show resolved Hide resolved
osu.Game/Overlays/News/Sidebar/NewsSideBar.cs Outdated Show resolved Hide resolved
@EVAST9919 EVAST9919 requested a review from bdach May 11, 2021 14:49
osu.Game/Overlays/News/Sidebar/YearsPanel.cs Outdated Show resolved Hide resolved
{
IdleColour = isCurrent ? Color4.White : colourProvider.Light2;
HoverColour = isCurrent ? Color4.White : colourProvider.Light1;
Action = () => { }; // Avoid button being disabled since there's no proper action assigned.
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in comment, we probably want this to do more than nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relies on integration since we will be calling a method in a NewsOverlay which will trigger a new request, which will reconstruct the whole side bar.

Copy link
Member

Choose a reason for hiding this comment

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

What I'm saying is that in the test scene this should be hooked up locally. So that it doesn't do nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be resolved in 586c5c7

osu.Game/Overlays/News/Sidebar/YearsPanel.cs Outdated Show resolved Hide resolved
@bdach
Copy link
Collaborator

bdach commented May 17, 2021

have applied further cleanups. seems passable for now.

bdach
bdach previously approved these changes May 17, 2021
@bdach bdach requested a review from peppy May 17, 2021 17:22
Comment on lines 180 to 194
private bool autoSizeTransitionApplied;

// Workaround to allow the dropdown to be opened immediately since FinishTransforms doesn't work for AutoSize{Duration,Easing}.
protected override void UpdateAfterAutoSize()
{
base.UpdateAfterAutoSize();

if (!autoSizeTransitionApplied)
{
AutoSizeDuration = animation_duration;
AutoSizeEasing = Easing.OutQuint;

autoSizeTransitionApplied = true;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what this is trying to achieve, but it looks fine without this logic?

Copy link
Member

Choose a reason for hiding this comment

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

Makes the posts container autosize immediately on first frame while applying autosize transforms on state changes afterwards.

For some reason this can't be done with a scheduled FinishTransforms, although I got to admit I haven't checked thoroughly why, will do in a bit.

Copy link
Collaborator

@bdach bdach May 18, 2021

Choose a reason for hiding this comment

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

I'm fine with removing. It's not like this looks bad if the section expand animation plays out on first load instead of the section showing instantly.

The reason this can't be done with a schedule is that schedules (both normal, and after children) run too early. Autosize is the last thing in a subtree update.

Copy link
Member

Choose a reason for hiding this comment

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

I did actually mean a 2-frame scheduling but yeah, what's the point at this point, let the animation play and call it a day.

@EVAST9919 EVAST9919 requested a review from peppy May 18, 2021 19:35
HoverColour = colourProvider.Light1;

TooltipText = "view in browser";
Action = () => host.OpenUrlExternally("https://osu.ppy.sh/home/news/" + post.Slug);
Copy link
Member

Choose a reason for hiding this comment

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

Just a heads up that the news system is the only placed these domains are hard-coded. It's going to need to be fixed at some point to use EndpointConfiguration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point, although I expect that action will be going away at some point anyway, for I expect news to be ultimately shown in the client when the implementation is complete.

Copy link
Member

Choose a reason for hiding this comment

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

Definitely, although I think we will still have "open on web" buttons like in user profile etc.

Anyways that's for another day, as these hard-coded urls are also in other classes this PR doesn't touch.

@peppy peppy merged commit 87833bf into ppy:master May 19, 2021
@EVAST9919 EVAST9919 deleted the news-sidebar-new branch May 19, 2021 10:56
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