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

Replace SCROLLABLE_ROW_HEIGHT define with constexpr kScrollableRowHeight #21906

Closed
wants to merge 11 commits into from

Conversation

katerid
Copy link

@katerid katerid commented Apr 26, 2024

Replacing SCROLLABLE_ROW_HEIGHT with constexpr and kScrollableRowHeight

Part of #21421

@katerid
Copy link
Author

katerid commented Apr 26, 2024

I believe that this should be ready for review, the only issue is that one of my commits had too short of a name.

Copy link
Member

@Gymnasiast Gymnasiast left a comment

Choose a reason for hiding this comment

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

There are a number of changes in this PR that shouldn’t be there, like one deleted file, two instances where the trailing newline was removed, and one case of an extra newline. Please fix these. The actual meat of the PR does look good, though.

Also, please do not close PRs only to recreate them - just update your existing one.

@@ -1,3 +1,4 @@

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change, please revert.

@@ -487,4 +487,4 @@ static void PrintLaunchInformation()
Console::WriteLine();

// TODO Print other potential information (e.g. user, hardware)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change, please revert.

@@ -84,4 +84,4 @@ struct NewVersionInfo
std::string url;
};

NewVersionInfo GetLatestVersion();
NewVersionInfo GetLatestVersion();
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change, please revert.

@@ -1,85 +0,0 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change, please revert.

@Gymnasiast Gymnasiast added the squash merge A PR that should be squashed on merge. label Apr 27, 2024
@katerid
Copy link
Author

katerid commented Apr 28, 2024

There are a number of changes in this PR that shouldn’t be there, like one deleted file, two instances where the trailing newline was removed, and one case of an extra newline. Please fix these. The actual meat of the PR does look good, though.

Also, please do not close PRs only to recreate them - just update your existing one.

Sorry about the closed PR, I was just changing which file I was working on and I'm still newer to github. I didn't know how to take out some of my old commits and only include the new ones that were relevant to this PR which was my goal with creating a new one.

@Gymnasiast
Copy link
Member

Gymnasiast commented Apr 28, 2024

No worries, we have all had to learn it.

Removing unwanted commits can be done using a rebase. It is an advanced operation though. For this PR, just make sure you get the diff (what is shown on the "Files changed" tab) to be correct - we’ll just squash all your commits into one.

A tip for future PRs: make sure you make your changes on a branch, rather than the master branch. This keeps things cleaner and avoids unrelated commits stacking up.

@tupaschoal tupaschoal changed the title Working on issue #21421 on file Window.h Replace constants that use #define with constexpr on Window.h Apr 30, 2024
@Gymnasiast
Copy link
Member

@katerid Could you address the line notes?

@AaronVanGeffen AaronVanGeffen changed the title Replace constants that use #define with constexpr on Window.h Replace SCROLLABLE_ROW_HEIGHT define with constexpr kScrollableRowHeight May 18, 2024
@AaronVanGeffen
Copy link
Member

I intended to rebase this PR, but it looks like the SCROLLABLE_ROW_HEIGHT define has already been replaced with a constexpr kScrollableRowHeight in #21760. Therefore closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squash merge A PR that should be squashed on merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants