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

IBX-6843: Focus mode #955

Merged
merged 6 commits into from
Nov 17, 2023
Merged

IBX-6843: Focus mode #955

merged 6 commits into from
Nov 17, 2023

Conversation

lucasOsti
Copy link
Contributor

@lucasOsti lucasOsti commented Oct 24, 2023

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-6843
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? yes
Doc needed? yes/no
License GPL-2.0

Related PR's

Normal view
Screenshot 2023-10-26 at 15 33 44

Focus mode view
Screenshot 2023-10-26 at 15 53 08

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review


doc.body.addEventListener(
ENABLE_FOCUS_MODE_EVENT_NAME,
() => doc.body.addEventListener('keydown', watchDisableFocusModeByKeyboard),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
() => doc.body.addEventListener('keydown', watchDisableFocusModeByKeyboard),
() => doc.body.addEventListener('keydown', watchDisableFocusModeByKeyboard, false),

Copy link
Contributor

Choose a reason for hiding this comment

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

We use it everywhere like this but u know @GrabowskiM its default false? 🧆

}
};
const watchDisableFocusModeByKeyboard = (event) => {
if (event.key === 'Escape' || event.keyCode === 27) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (event.key === 'Escape' || event.keyCode === 27) {
if (event.key === 'Escape') {

@@ -270,7 +270,6 @@

{%- block richtext_widget -%}
{% set attr = attr|merge({ 'hidden': true }) %}

Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary deletion :)

</svg>
{%- endset -%}
{%- set title -%}
{{ 'focus_mode.disbale_hint'|trans({ '%icon%': title_icon|raw })|desc('To exit focus mode, click the %icon% or press Esc')|raw }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{ 'focus_mode.disbale_hint'|trans({ '%icon%': title_icon|raw })|desc('To exit focus mode, click the %icon% or press Esc')|raw }}
{{ 'focus_mode.disable_hint'|trans({ '%icon%': title_icon|raw })|desc('To exit focus mode, click the %icon% or press Esc')|raw }}


.ibexa-field-edit {
&__focus-mode {
margin: auto 0 0 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
margin: auto 0 0 0;
margin: auto 0 0;

Comment on lines 3 to 4
const ENABLE_FOCUS_MODE_EVENT_NAME = 'ibexa-focus-mode:on';
const DISABLE_FOCUS_MODE_EVENT_NAME = 'ibexa-focus-mode:off';
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe?

Suggested change
const ENABLE_FOCUS_MODE_EVENT_NAME = 'ibexa-focus-mode:on';
const DISABLE_FOCUS_MODE_EVENT_NAME = 'ibexa-focus-mode:off';
const FOCUS_MODE_ON_EVENT = 'ibexa-focus-mode:on';
const FOCUS_MODE_OFF_EVENT = 'ibexa-focus-mode:off';
Suggested change
const ENABLE_FOCUS_MODE_EVENT_NAME = 'ibexa-focus-mode:on';
const DISABLE_FOCUS_MODE_EVENT_NAME = 'ibexa-focus-mode:off';
const FOCUS_MODE_ENABLE_EVENT = 'ibexa-focus-mode:enable';
const FOCUS_MODE_DISABLE_EVENT = 'ibexa-focus-mode:disable';

@@ -0,0 +1,61 @@
(function (global, doc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe stick to one logic enable/disable || on/off?

activeFieldEdit = null;
}
};
const watchDisableFocusModeByKeyboard = (event) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we always add Keyboard with func name for keydown but never Click when using click? 😄
Maybe exitFocusMode || exitFocusModeByKeyboard

overflow-y: auto;
align-self: stretch;
flex-grow: 1;
padding: calculateRem(8px) calculateRem(32px) calculateRem(32px) calculateRem(32px);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
padding: calculateRem(8px) calculateRem(32px) calculateRem(32px) calculateRem(32px);
padding: calculateRem(8px) calculateRem(32px) calculateRem(32px);

Copy link

sonarcloud bot commented Nov 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@micszo micszo left a comment

Choose a reason for hiding this comment

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

QA Approved on Ibexa Commerce 4.6.x-dev.

@micszo micszo removed their assignment Nov 17, 2023
@dew326 dew326 merged commit 5be86ac into main Nov 17, 2023
23 checks passed
@dew326 dew326 deleted the IBX-6843-focus-mode branch November 17, 2023 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants