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

fix: remove the minimum width to truncate text on richcombo select #195

Merged
merged 2 commits into from
Mar 23, 2022

Conversation

diegonvs
Copy link
Contributor

@diegonvs diegonvs commented Feb 11, 2022

Why?

It was necessary to make texts like: "Headers: First Column" fit inside richcombo select without truncating

Before:

Screen Shot 2022-02-11 at 19 33 27

After

Screen Shot 2022-02-11 at 19 33 44

Copy link
Member

@markocikos markocikos left a comment

Choose a reason for hiding this comment

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

@diegonvs see comment inline.

@@ -293,7 +293,7 @@ a.cke_combo_button {
float: left;
cursor: default;
color: #6b6c7e; /* $secondary */
width: 60px;
width: 130px;
Copy link
Member

Choose a reason for hiding this comment

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

Setting width to a fixed value, in my experience, is rarely the best solution. Truncating may happen again in a different locale, and hardcoding large width may look weird and break on small resolutions. It's typically best to have something like 100% width and fixed max-width. Or change layout to flex instead of using floats. Maybe @edalgrin can give us some tips here.

Choose a reason for hiding this comment

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

@markocikos is right, setting the width to a fixed value might be limiting in other situations, but that's also true for the other proposals. Considering mobile scenarios with width of 320px or select options larger than 130px (due to customizations or translations). Ideally, we shouldn't truncate the select at any point and rather split the toolbar in 2 lines when it reaches the browser (or editor) limit. Another solution would be to group the secondary options inside a new icon on mobile (Lexicon and our Squad were working on an alternative solution for the management-toolbar component but it doesn't seem to be ready yet) so my recommendation is to take the first solution and remove the width value completely. If everything is right we should obtain the desired result by default (POC: https://codepen.io/eduardoallegrini/pen/NWwaKjj)

Copy link
Contributor Author

@diegonvs diegonvs Feb 14, 2022

Choose a reason for hiding this comment

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

my recommendation is to take the first solution and remove the width value completely

When removing width on small screens it will look like:

Screen Shot 2022-02-14 at 17 18 49

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This solution looks fine but I would hear more from a design perspective.

Calling the lexicon rangers 📞 : @drakonux @emiliano-cicero

Copy link
Member

Choose a reason for hiding this comment

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

Hello everyone,

We agree with @edalgrin that the first solution is the most feasible.

Some doubts:

  • The margin between the left edge and the dropdown / row dropdown must be 8px
  • The icon-caret-down-l seems different in the header dropdown to the ones shown for the row/column/cell dropdowns.
  • Besides the alloy editor design doesn't show carets for those row/column/cell dropdowns (They could)
  • The balloon width doesn't has to reach the right edge.

Frame 699

Copy link
Contributor Author

@diegonvs diegonvs Feb 15, 2022

Choose a reason for hiding this comment

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

The margin between the left edge and the dropdown / row dropdown must be 8px

The icon-caret-down-l seems different in the header dropdown to the ones shown for the row/column/cell dropdowns.

Besides the alloy editor design doesn't show carets for those row/column/cell dropdowns (They could)

The balloon width doesn't has to reach the right edge.

I spent a time trying to fix these issues and it will take more time than I thought. Can you create more detailed images about the issues and send to me to create new tickets for it?

Copy link
Member

Choose a reason for hiding this comment

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

I'll create a design ticket for it @diegonvs . However, it would be great to reproduce the actual implementation in my local DXP. Could you give me the steps to get the use case you are fixing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you give me the steps to get the use case you are fixing?

For sure.

We can have the easy way[1] and the correct way[2]:
both ways assume that you aren't using liferay running on docker

For 1) *It will not require all the work we should do with [2]

  1. gw clean deploy on frontend-editor-sample-ckeditor-web module
  2. Go to Page Editor and add a widget called CKEditor Sample on the page
  3. See it rendering and open Chrome DevTools
  4. Click on the editor area and then click on insert button and create a table from it
  5. Search for cke_combo_text inside "Elements" panel (See the attached video)
  6. Remove the width property inside it and voìla
Screen.Recording.2022-02-18.at.18.22.58.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For 2)

  1. Clone this repo and fetch this PR
  2. Run yarn install inside this repo
  3. Run ./ck.sh setup inside this repo
  4. Run ./ck.sh build inside this repo

Nice, we have now the artifacts to be updated on DXP.

On DXP side:

  1. Go to modules/ folder
  2. Run rm -rf node_modules/liferay-ckeditor && cp -R PATH_TO_CLONED_LIFERAY_CKEDITOR node_modules/
  3. Navigate to frontend-editor-ckeditor-web module
  4. Run gw clean deploy
  5. Get back to frontend-editor-ckeditor-sample-web module and gw clean deploy it too
  6. Go to Page Editor and add a widget called CKEditor Sample on the page
  7. Publish the page and see it working(at least the styles because it will not work completely because you'll need to fetch changes from LPS-146407 Bring table headers functionallity to BalloonEditor liferay-frontend/liferay-portal#1910 and I don't want to add more complexity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notes:

  1. We should have control of DXP source code. I can't see it working or how it can be bundled on a docker image for example
  2. My dream ™️ is one day have deploy previews as we have on Clay but on DXP side it's impossible and we don't own the tooling of this repo and the code necessary to make BalloonEditor work is distributed between this repo and DXP source code due to other reasons. /cc @markocikos

Instead of truncating it will just place the select in a line above of other toolbar elements. See liferay#195 (comment) for further details

It is necessary to make texts like: "Headers: First Column" fit inside richcombo select without truncating and get it readable
This commit autogenerated with "./ck.sh".
@diegonvs diegonvs changed the title fix: increase the minimum width to truncate text on richcombo select fix: remove the minimum width to truncate text on richcombo select Feb 14, 2022
Copy link
Member

@markocikos markocikos left a comment

Choose a reason for hiding this comment

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

LGTM, as a temporary fix.

@diegonvs diegonvs merged commit 8400888 into liferay:master Mar 23, 2022
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