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.

Sign up for GitHub

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

Jump to bottom

[#2127] Align right panel with welcome title #2229

Merged
merged 6 commits into from
Jul 24, 2024
Merged

[#2127] Align right panel with welcome title #2229

merged 6 commits into from
Jul 24, 2024

Conversation

jedkohjk
Copy link
Contributor

@jedkohjk jedkohjk commented Jun 30, 2024 •
edited by ckcherry23
Loading

Resolves #2127

Proposed commit message

Align panel titles

The title text on the left panel is not flushed to the top because it
is a header.

There are several types of title texts on the right panel. Initially,
some were headers and others were not. There was padding to ensure that
those which were not headers were not flushed to the top.  However, this
meant that those headers were lower than expected and not aligned. 

To align the texts, let us remove the padding for the right panel and
set the first element on the right panel as a header.

Other information

Before:

Before

After:

After

ckcherry23
ckcherry23 reviewed Jun 30, 2024
View reviewed changes
Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

Hi @jedkohjk, thanks for working on this.

In issue #2127, the 'report title' we refer to is the custom title specified by the user for the top of the left panel, which you can specify using this: https://reposense.org/ug/customizingReports.html#add-a-title. Please use an H1 report title for testing.



Consequently, we would like the right panel's 1. Welcome text, 2. 'Code Panel', and 3. 'Commit Panel' to be top-aligned with the left panel's Report Title. This means the right panel headings will move upwards and will also allow for lesser scrolling.

Please also ensure that the CI passes after you make your changes. Thank you!

@jedkohjk jedkohjk requested a review from ckcherry23 July 1, 2024 04:18
ckcherry23
ckcherry23 reviewed Jul 1, 2024
View reviewed changes
Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

Thank you @jedkohjk for your first contribution and the preview screenshots! I have a few things to clarify.

Also, the macOS 11 CI is failing as recorded in #2231. I believe that issue must be fixed before we can merge any PRs.

frontend/src/styles/panels.scss Outdated
@@ -2,7 +2,7 @@
@import 'z-indices';

.panel-padding {
padding: 2rem 1.5rem 2rem 2.2rem;
padding: 0 0 2rem 2.2rem;
Copy link
Member

Choose a reason for hiding this comment

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

I think we can keep the right padding?

Suggested change
padding: 0 0 2rem 2.2rem;
padding: 0 1.5rem 2rem 2.2rem;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

frontend/src/views/c-authorship.vue
v-on:click="toggleAllFileActiveProperty(true)"
) show all file details
a(v-if="activeFilesCount > 0", v-on:click="toggleAllFileActiveProperty(false)") hide all file details
h2
Copy link
Member

Choose a reason for hiding this comment

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

Was this h2 added to provide padding or for semantic reasons? Because the .toolbar--multiline wouldn't be a heading right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the h2, the Code Panel will appear at the top of the screen, above the title.

without h2

If .toolbar--multiline is not included in h2, it appears on the line below instead.

not in h2

The expected behaviour is as follows:

expected

Copy link
Member

Choose a reason for hiding this comment

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

It may be possible to align by adding top padding for these elements instead of making them both h2, but this works well enough.

ckcherry23
ckcherry23 approved these changes Jul 2, 2024
View reviewed changes
Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ckcherry23 ckcherry23 requested a review from a team July 2, 2024 18:03
jonasongg
jonasongg approved these changes Jul 4, 2024
View reviewed changes
Copy link
Contributor

@jonasongg jonasongg left a comment

Choose a reason for hiding this comment

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

LGTM!

@damithc
Copy link
Collaborator

damithc commented Jul 24, 2024

Folks, any update on this PR?

@gok99
Copy link
Contributor

gok99 commented Jul 24, 2024

@jedkohjk Could you add a brief commit message explaining the changes made, following the guidlines here or other merged PRs?

@jedkohjk
Copy link
Contributor Author

@jedkohjk Could you add a brief commit message explaining the changes made, following the guidlines here or other merged PRs?

Updated the proposed commit message

@ckcherry23
Copy link
Member

Thanks for the update, @jedkohjk. We can also use a line break tool to auto-wrap commit messages at 72 characters.

@ckcherry23 ckcherry23 merged commit dd0c13e into reposense:master Jul 24, 2024
8 checks passed
@github-actions GitHub Actions
Copy link
Contributor

The following links are for previewing this pull request:

@jedkohjk jedkohjk changed the title Align right panel with welcome title [#2127] Align right panel with welcome title Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckcherry23 ckcherry23 ckcherry23 approved these changes

@jonasongg jonasongg jonasongg approved these changes

Assignees
No one assigned
Labels
s.ToMerge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Align report title with right panel
5 participants
@jedkohjk @damithc @gok99 @ckcherry23 @jonasongg