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

[FEATURE]: Add default page size setting #3498

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

5e-Cleric
Copy link
Member

Opened this as an upstream PR by a miss click, but shouldn't be a difference

This feature does not check for account, unlike the default save location, because it is not deemed so important.

Checks the new page style code for a page size, if a page size is already defined, it doesn't add anything. if there isn't and the default is A4, adds the A4 snippet via simple concatenation.

@5e-Cleric 5e-Cleric added the 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed label May 27, 2024
@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3498 May 27, 2024 21:06 Inactive
@5e-Cleric 5e-Cleric changed the title feat:default page size [FEATURE]: Add default page size setting May 27, 2024
@5e-Cleric 5e-Cleric added the P2 - minor feature or not urgent Minor bugs or less-popular features label May 27, 2024
@5e-Cleric 5e-Cleric mentioned this pull request May 27, 2024
@5e-Cleric
Copy link
Member Author

To test:

  • Login in deployment using your session cookie
  • Navigate to your new page, notice if the page size is changed in the style tab
  • Navigate to Account Page
  • Change default size to A4
  • Navigate back to new page, now page size will be set to A4 and the A4 page size snippet will be inserted at the top of the CSS

@5e-Cleric
Copy link
Member Author

5e-Cleric commented May 27, 2024

Thinking about it, i probably should insert the snippet at the end of the CSS code, otherwise, @import won't work.

@5e-Cleric 5e-Cleric temporarily deployed to homebrewery-pr-3498 May 27, 2024 21:12 Inactive
@Gazook89
Copy link
Collaborator

If this is just adding a snippet automatically when you go to /new/, why would there be an @import in there already? Unless you are doing "clone to new", the only thing that would be in the Style Editor at the beginning is the default example CSS.

What happens if someone does "clone to new"? Presumably you wouldn't want to clone to new and then find that the document page size has been adjusted to your preference, right?

@5e-Cleric
Copy link
Member Author

If this is just adding a snippet automatically when you go to /new/, why would there be an @import in there already? Unless you are doing "clone to new", the only thing that would be in the Style Editor at the beginning is the default example CSS.

What happens if someone does "clone to new"? Presumably you wouldn't want to clone to new and then find that the document page size has been adjusted to your preference, right?

It has been some time since i made this, but i believe i made it pretty secure to make sure i don't invalidate any import or font face statement and still works, i would have to check to be sure. Requires testing.

Also, i consider this a p3 priority at most.

@Gazook89
Copy link
Collaborator

Gazook89 commented Sep 16, 2024

Just testing this for a second, I noticed that it is dropping 'undefined' in the first line when I create new from blank:

Output:

undefined

/* A4 Page Size */
.page{
    width  : 210mm;
    height : 296.8mm;
}

Edit

This feature does not check for account, unlike the default save location, because it is not deemed so important.

I think it's worth more consideration that this should be an account setting, rather than just localStorage. It's difficult to determine the current page size just by visually glancing at the Preview pane, and considering how often I have to tell users where the Style editor is, I think there are plenty of people who won't even check in there to see the page size.

And if a user is writing across multiple computers, it's a little weird.

Finally, the setting is currently located on the Account page. It seems a weird distinction to have some of these settings be set in localStorage and some to their actual account.


Rather than using regex in the editor, would it be better to drill into the iframe and find the actual applied/calculated values of the height and width properties of the .page and determine if they've been changed from the default? This might be less brittle than regex, though likely more difficult to do (but I'm not sure how much more).


Little more testing

  • When cloning A4 sized brew with user default US Letter, it retains the A4
  • When cloning US Letter sized brew with user default A4, it retains US Letter
  • Radio Inputs on Account page display the current setting across page reloads
  • Consistent styling: The radio inputs should match the radio inputs of the default save location buttons directly above this.

@5e-Cleric
Copy link
Member Author

I should probably move this to be inserted not into the editor but somewhere else, maybe in the blank theme?

It makes more sense, not sure why i did it this way.

@Gazook89
Copy link
Collaborator

I'm not sure what you mean about putting it into the Blank theme? If you mean you would alter the page size via the Blank theme files somehow, I am not sure that is the best route (or at least necessary at this time). We shouldn't want to hide the fact that the page size is getting altered from our default-- adding the actual text of the snippet into the Style Editor where it is visible to users is good. It tells them that something happened, and is a good signal to them that the dimensions can be further changed.

Maybe in the future the page dimensions could be changed via a menu in the Properties Editor, but I'm not sure there is any advantage to that.

I don't know if my previous comments are what you inspired your response-- but if so, I should be clear that I don't think there is anything wrong with inserting the snippet in the Style editor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 - minor feature or not urgent Minor bugs or less-popular features 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

3 participants