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

feat: added pictures align features #128

Closed
wants to merge 6 commits into from
Closed

feat: added pictures align features #128

wants to merge 6 commits into from

Conversation

baravkareknath
Copy link
Contributor

@baravkareknath baravkareknath commented Oct 30, 2023

Description

Related resources

  • #...
  • #...

Checklist

  • I have opened this pull request against master
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on
    Slack to find a “pr review buddy” who is going to review my pull request.

root and others added 4 commits October 30, 2023 10:03
Signed-off-by: eknath.baravkar <eknath.baravkar@infosys.com>
Signed-off-by: eknath.baravkar <eknath.baravkar@infosys.com>
@baravkareknath baravkareknath changed the title Added pictures align features feat: added pictures align features Oct 30, 2023
@baravkareknath
Copy link
Contributor Author

Update PR title please review contribution

@fsbraun
Copy link
Member

fsbraun commented Oct 30, 2023

@baravkareknath Please, can you add a structured description of what this PR is supposed to do?

So far, I see you have changed the alignment options in the model. How do the new options differ from the existing ones. How are these options rendered?

@baravkareknath
Copy link
Contributor Author

Hi fsbraun thanks for your replay.
I am not changed the old alignment options in the model. Just added new options that are works like old options. New options are for float images from left to right and vice versa and set image on top and bottom. Looks as below-

Add setting for picture alignment, renders a class or inline styles

depending on your template setup

def get_alignment():
alignment = getattr(
settings,
'DJANGOCMS_PICTURE_ALIGN',
(
('left', _('Align left')),
('right', _('Align right')),
('center', _('Align center')),
('ltr', _('Left-to-Right')), #Newly added option
('rtl', _('Right-to-Left')), #Newly added option
('top', _('Top Aligned')), #Newly added option
('bottom', _('Bottom Aligned')), #Newly added option
)
)
return alignment

Hopes that's options are also useful for picture alignment.

@baravkareknath
Copy link
Contributor Author

Hi fsbraun,
Please look into patch of code and let me know about this contribution.

@fsbraun
Copy link
Member

fsbraun commented Nov 10, 2023

@baravkareknath I still do not see how those options are rendered. As far as I can see, the PR adds options to the plugin which do not change its behaviour.

To me, it seems a cleaner solution to provide both the DJANGOCMS_PICTURE_ALIGN setting and accompanying CSS on a per-project basis.

I fear the project itself cannot make assumptions about the CSS used. Any idea on how to render those options including top and bottom alignment?

@baravkareknath
Copy link
Contributor Author

Hi fsbraun, As per your suggestion I will add CSS file in the same and share new patch of CSS.

Signed-off-by: eknath.baravkar <eknath.baravkar@infosys.com>
Signed-off-by: eknath.baravkar <eknath.baravkar@infosys.com>
@fsbraun
Copy link
Member

fsbraun commented Dec 7, 2023

Hi @baravkareknath !

I am not sure if this leads to any significant value add:

  • Unlike djangocms-frontend, djangocms-picture cannot and should not make any assumptions about the use of any CSS framework, like Bootstrap.
  • The alternative of using inline styling seems awkward and like you, I would not go down that road.

Adding the new align options can easily be done in a project's settings file, where no-one needs to guess what CSS classes are available.

Maybe it is enough to add an example to the README.rst in a separate PR where you would describe how to add floating images and what CSS you would need for it?

@baravkareknath
Copy link
Contributor Author

Hi fsbraun,
As per your suggestion i am pull new request and wants to submit it but Invalid PR title error occurred so can you suggest me which PR Title like docs, style or any others is suitable for "Added image align options example."

@baravkareknath baravkareknath closed this by deleting the head repository Jan 2, 2024
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.

2 participants