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

Update [email protected] and [email protected] #1134

Draft
wants to merge 99 commits into
base: master
Choose a base branch
from

Conversation

fel1x-developer
Copy link
Contributor

@fel1x-developer fel1x-developer commented Mar 12, 2024

Screenshot 2024-03-12 141915
Screenshot 2024-03-12 141948
Screenshot 2024-03-12 142002
Screenshot 2024-03-12 142011
Screenshot 2024-03-12 142024
Screenshot 2024-03-12 142033
Screenshot 2024-03-12 142041

@bdrewery
Copy link
Member

I want to merge this but the last commit is hard to review and has unrelated changes in it. Can you either remove the last commit or split it into coherent independent commits? "Use try/catch", "Split into several files". I don't like the splitting of the file as it makes review/history hard.

@fel1x-developer
Copy link
Contributor Author

I want to merge this but the last commit is hard to review and has unrelated changes in it. Can you either remove the last commit or split it into coherent independent commits? "Use try/catch", "Split into several files". I don't like the splitting of the file as it makes review/history hard.

I was working on other PR which aims for splitting js files using modules, and I made a mistake to add new modules files in this review. I removed them from the last commit.

});
$("#latest_url").attr("href", build_url(page_mastername, "latest"));
} else if (server_style == "inline") {
Copy link
Member

Choose a reason for hiding this comment

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

Why remove server_style support? It's not clear in the commits what's going on here. Restyling the file and removing logic makes it hard to review. I'm starting to think of the XZ hack with these large changes. Make them reviewable please.
See dfe3002

Copy link
Contributor Author

Choose a reason for hiding this comment

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

server_style variable has been renamed to serverStyle following camal case convention. (most other variable names too) Right now, server_style is defined in each html file before including js scripts, but this is a bad practice since we have to edit three files to change one variable. So I defined it in the beginning of poudriere.js and renamed server_style as serverStyle

Please check line 30 of modified poudriere.js

@fel1x-developer
Copy link
Contributor Author

I'll introduce these changes gradually

@fel1x-developer fel1x-developer marked this pull request as draft June 29, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants