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.

Sign up for GitHub

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

Jump to bottom

LPD-2001 - c-prefers-expanded-text utility breaks styling in page editor #689

Closed
wants to merge 1 commit into from
Closed

LPD-2001 - c-prefers-expanded-text utility breaks styling in page editor #689

wants to merge 1 commit into from

Conversation

ilzamcmed
Copy link
Collaborator

@ilzamcmed ilzamcmed commented Aug 13, 2024 •
edited
Loading

Jira ticket: https://liferay.atlassian.net/browse/LPD-2001

simplescreenrecorder-2024-08-19_14.32.05.mp4

@liferay-continuous-integration
Copy link
Collaborator

The following guidelines have been set by the owner of this repository:

  •     "Thank you for sending this PR to liferay-platform-experience. If you haven't already, take this opportunity to review our pull request guidelines. For additional help, please reach out via the appropriate channels."

@liferay-continuous-integration
Copy link
Collaborator

To conserve resources, the PR Tester does not automatically run for every pull.

If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed.

If your pull was never tested, comment "ci:test" to run the PR Tester for this pull.

@ilzamcmed
Copy link
Collaborator Author

ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: e01b93bb7dad6b4f4447b8b80d078569da21a849

Sender Branch:

Branch Name: LPD-2001
Branch GIT ID: d10304988954c1ebfa618fccf177e7544db19efe

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@ilzamcmed
Copy link
Collaborator Author

ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

@ilzamcmed
Copy link
Collaborator Author

Hello everyone, @pat270 submitted this PR to address the problem; however, there was no unanimous agreement on the solution. As a result, I've made the fix on the DXP side to resolve this specific issue.

@ilzamcmed
Copy link
Collaborator Author

ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 27 out of 27 jobs passed

✔️ ci:test:relevant - 47 out of 47 jobs passed in 1 hour 29 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: e01b93bb7dad6b4f4447b8b80d078569da21a849

Upstream Comparison:

Branch GIT ID: e01b93bb7dad6b4f4447b8b80d078569da21a849
Jenkins Build URL: EE Development Acceptance (master) - 734 - 2024-08-13[00:40:42]

ci:test:stable - 27 out of 27 jobs PASSED
27 Successful Jobs:
ci:test:relevant - 47 out of 47 jobs PASSED
47 Successful Jobs:
For more details click here.
Test bundle downloads:

@liferay-continuous-integration
Copy link
Collaborator

@ilzamcmed
Copy link
Collaborator Author

ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

❌ ci:test:sf - 0 out of 1 jobs passed in 3 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 4ac2bb325f95f17c29dd79238837f75e9d9a886e

Sender Branch:

Branch Name: LPD-2001
Branch GIT ID: 404837c33f3cab22907bfec4601abfaae97fe427

0 out of 1jobs PASSED
For more details click here.
     [exec] > Task :packageRunCheckFormat
     [exec] yarn run v1.13.0
     [exec] \$ node-scripts check:ci
     [exec] 
     [exec] ⚙️ Running preflight checks...
     [exec] 
     [exec] ⚙️ Checking outdated tsconfig.json files ...
     [exec] 
     [exec] ⚙️ Running TypeScript checks on modified files...
     [exec] ℹ️ A total of 12 CPUs were detected: launching tsc using 12 workers
     [exec] 
     [exec] ⚙️ Running format checks on modified files...
     [exec]    /opt/dev/projects/github/liferay-portal/modules/apps/layout/layout-content-page-editor-web/src/main/resources/META-INF/resources/page_editor/app/components/ItemConfiguration.js
     [exec]      1:1  error  File has format errors.  (format check)
     [exec]    
     [exec]    ✖ 1 problem (1 error, 0 warnings)
     [exec]    
     [exec]    
     [exec] ❌ CI checks failed.
     [exec] error Command failed with exit code 1.
     [exec] info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
     [exec] 
     [exec] > Task :packageRunCheckFormat FAILED
     [exec] Gradle build finished at 2024-08-14 12:31:09.180.
     [exec] 
     [exec] 
     [exec] FAILURE: Build failed with an exception.
     [exec] 
     [exec] * What went wrong:
     [exec] Execution failed for task ':packageRunCheckFormat'.
     [exec] > Process 'command '/opt/dev/projects/github/liferay-portal/build/node/bin/node'' finished with non-zero exit value 1
     [exec] 
     [exec] 3 actionable tasks: 2 executed, 1 up-to-date* Try:
     [exec] 
     [exec] 
     [exec] > Run with --info or --debug option to get more log output.
     [exec] > Run with --scan to get full insights.
     [exec] See the profiling report at: file:///opt/dev/projects/github/liferay-portal/build/reports/profile/profile-2024-08-14-05-30-58.html> Get more help at https://help.gradle.org.
     [exec] A fine-grained performance profile is available: use the --scan option.
     [exec] 
     [exec] * Exception is:
     [exec] org.gradle.api.tasks.TaskExecutionException: Execution failed for task ':packageRunCheckFormat'.
     [exec] 
     [exec]   at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.lambda\$executeIfValid\$1(ExecuteActionsTaskExecuter.java:148)
     [exec]   at org.gradle.internal.Try\$Failure.ifSuccessfulOrElse(Try.java:282)
     [exec]   at org.gradle.api.internal.tasks.execution.ExecuteActionsTaskExecuter.executeIfValid(ExecuteActionsTaskExecuter.java:146)

@liferay-continuous-integration
Copy link
Collaborator

@ilzamcmed
Copy link
Collaborator Author

I am getting some errors while running gw formatSource. Investigating...

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 4 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 4da60e29fa13a98180e553f79b398d7c46634b41

Sender Branch:

Branch Name: LPD-2001
Branch GIT ID: feab2ba03e55735cdec092b613600069bcf3e638

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

@ilzamcmed
Copy link
Collaborator Author

ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 27 out of 27 jobs passed

✔️ ci:test:relevant - 47 out of 47 jobs passed in 59 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 4da60e29fa13a98180e553f79b398d7c46634b41

Upstream Comparison:

Branch GIT ID: 2723417e0e6fc75711d921b2f6699531e7ba3ade
Jenkins Build URL: EE Development Acceptance (master) - 756 - 2024-08-20[00:43:16]

ci:test:stable - 27 out of 27 jobs PASSED
27 Successful Jobs:
ci:test:relevant - 47 out of 47 jobs PASSED
47 Successful Jobs:
For more details click here.
Test bundle downloads:

@liferay-continuous-integration
Copy link
Collaborator

@ilzamcmed
Copy link
Collaborator Author

Hey @pat270 and @ethib137
This PR is ready for review, again 😄
Thanks!

@ethib137
Copy link
Collaborator

Thanks @ilzamcmed . In this case, I don't think we want to take this approach though. We shouldn't need a one off fix in a single component for this. Whatever our fix is it should be more global and ideally take place in Clay. I think the only fix we may need would be removing max-width: 100% !important; from c-prefers-expanded-text.

@pat270 is this really needed. In some basic testing it looks to me like it shouldn't be necessary.

Screenshot 2024-08-21 at 2 15 16 PM

@ilzamcmed
Copy link
Collaborator Author

Thanks @ethib137! I thought this solution to be more of a specific change since the one we should fix on clay didn't have any definition with @marcoscv-work on the other PR.
But you are correct! I Agree with a more global fix.

Removing the max-width: 100% !important; makes the text break into a new line. Is that ok?

image

@pat270
Copy link
Collaborator

pat270 commented Aug 21, 2024 •
edited
Loading

@ethib137 We can get rid of max-width: 100%. The reason why it's there is because truncate-text changes the display property to block, originally it is inline. c-prefers-expanded-text doesn't change it back to inline. If we overwrite to inline, we can remove the 100%. I'm not sure why I did it that way. I can't seem to find the edge case.

This one gets tricky because of the flex-nowrap customization on nav-tabs. We need to remove that. The tabs will break to new line when expanded like this:

will-break-to-new-line

Edit: I found the edge case. Changing text-truncate from block to inline will cause the height of the tab to become shorter. The max-width: 100% is needed to avoid that. I think we should remove flex-nowrap and remove c-inner from tabs.

We don't need c-inner anymore due to focus-visible support. See https://clayui.com/docs/css/utilities/c-inner.html.

Edit 2: Maybe we should consider removing c-inner from DXP.

@ethib137
Copy link
Collaborator

This is looking closer to the right solution. Our tabs on multiple lines does not look good. That probably needs to be rethought from a design perspective.

@pat270 what's the problem with just removing max-width: 100% and not changing the display?

Also, it looks like cadmin is overriding c-prefers-expanded-text.

In terms of removing c-inner from dxp, that's probably a good idea to create a ticket for it and add it to the backlog.

@pat270
Copy link
Collaborator

pat270 commented Aug 22, 2024

@ethib137 @marcoscv-work gave us some styling options when tabs break to new line at liferay/clay#5622. The difficult part about this is targeting the styles to apply only when tabs have broke to new line. I tried to use container queries today, but it didn't get it to work yet. I think I might be able to get it to work with grid? The problem with container queries is that we need a fixed or percentage width on the element.

what's the problem with just removing max-width: 100% and not changing the display?

display: inline makes some content shift vertically. This gif should explain better.

display-inline

Also, it looks like cadmin is overriding c-prefers-expanded-text.

c-prefers-expanded-text uses important on the properties.

@ethib137
Copy link
Collaborator

Hey @pat270 your gif looks like you're adding display: inline. I'm suggesting just remove max-width nothing else.

Screen.Recording.2024-08-23.at.2.57.56.PM.mov

@pat270
Copy link
Collaborator

pat270 commented Aug 23, 2024

@ethib137 You're right, we can remove it. It doesn't impact anything. Are you ok with the text just breaking to new line?

@ethib137
Copy link
Collaborator

Yeah, I think text breaking to new line is fine. In most cases that's what you want. At some point we can work on a tabs improvement to fix that, but it's not high priority.

@ilzamcmed
Copy link
Collaborator Author

ilzamcmed commented Aug 23, 2024 •
edited
Loading

To summarize:

  1. Clay side: remove max-width: 100%
  2. DXP side:
    • undo the changes made on this PR
    • create a task to remove c-inner class.

@ethib137 @pat270, can I proceed ith these changes?

@pat270
Copy link
Collaborator

pat270 commented Aug 23, 2024

@ilzamcmed I can make the change on Clay's side. I'm still trying to figure out how to switch the styles when tabs break to new line.

@pat270 pat270 mentioned this pull request Aug 23, 2024
Merged
@pat270
Copy link
Collaborator

pat270 commented Aug 23, 2024

@ilzamcmed we don't need to remove c-inner. It will work fine with what Evan suggested. I can create a ticket for removing c-inner as a whole.

@ethib137
Copy link
Collaborator

Okay.. so we should be able to close this PR then right?

@ilzamcmed
Copy link
Collaborator Author

Thanks guys! 🎉

@ilzamcmed ilzamcmed closed this Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethib137 ethib137 Awaiting requested review from ethib137

@pat270 pat270 Awaiting requested review from pat270

Assignees
No one assigned
Labels
ci:test:relevant - success ci:test:sf - success ci:test:stable - success
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ilzamcmed @liferay-continuous-integration @ethib137 @pat270