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

Batch of fixes #262

Merged
merged 16 commits into from
May 14, 2023
Merged

Batch of fixes #262

merged 16 commits into from
May 14, 2023

Conversation

gagnieray
Copy link
Contributor

@gagnieray gagnieray commented Mar 21, 2023

In hope this PR will help you keep this awesome theme working with newer versions of Redmine 🤞 🚀

Fixes issues #222, #233, #242, #245, #246, #253, and few others not already reported 🐛 🩹

Tested on Redmine 4.2.10 and 5.0.5 with the following plugins enabed :

@felixsinger
Copy link

Awesome. Thanks!

@simon-isler
Copy link

Nice! @mrliptontea would you have time to look into this?

Copy link
Owner

@mrliptontea mrliptontea left a comment

Choose a reason for hiding this comment

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

I am not able to check these changes myself, broadly they look OK. However, issues I see:

  • Some of these changes touch basic Redmine layout - were these changes tested in:
    • Different configurations? E.g. fixed layout true/false.
    • Earlier Redmine versions? Although I guess it shouldn't matter as much, they're old.
  • Few places need to use variables instead of hard-coded values (e.g. icons) for improved readability and consistency.

I also need to make linting GitHub action work.

src/sass/components/_content.scss Outdated Show resolved Hide resolved
Comment on lines 8 to 17
transform: translateY(-175%);
@if $sidebar-position == "right" {
right: 15px;
}
}

.flyout-menu .dashboard-system-default {
position: absolute;
right: 10px;
transform: translateY(-190%);
Copy link
Owner

Choose a reason for hiding this comment

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

This (and -175% above) are a bit specific/odd values. Do they work correctly on different window sizes? If yes - can you add a comment and explain why these values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this works on different windows sizes, but it doesn't when changing some variables.

The icon added by the plugin is outside the a tag used to draw the tab. So I solved this by computing existing variables. It should be more explicit.

src/sass/components/_plugins.scss Outdated Show resolved Hide resolved
src/sass/components/_plugins.scss Outdated Show resolved Hide resolved
@gagnieray
Copy link
Contributor Author

gagnieray commented May 11, 2023

  • Some of these changes touch basic Redmine layout - were these changes tested in:
    • Different configurations? E.g. fixed layout true/false.
    • Earlier Redmine versions? Although I guess it shouldn't matter as much, they're old.

Yes I tried to test these changes in different configurations : fixed layout + css-grid-layout/$flexbox-layout + sidebar-position.

Regarding earlier Redmine versions, I only tested on the 2 latest releases.

Copy link
Owner

@mrliptontea mrliptontea left a comment

Choose a reason for hiding this comment

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

Nice job!

@mrliptontea mrliptontea merged commit 50919b2 into mrliptontea:master May 14, 2023
This was referenced May 15, 2023
@gagnieray gagnieray deleted the feature/fix-master branch November 21, 2023 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants