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

refactor: change type and methods in MediaRange.ts #6120

Merged
merged 8 commits into from
Dec 6, 2022
Merged

refactor: change type and methods in MediaRange.ts #6120

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7673730
chore: add type to FocusableElements
hinzzx Nov 28, 2022
d92688d
chore: code review change
hinzzx Nov 28, 2022
31d2ce9
chore: better type name and startFromContainer param set to optional
hinzzx Nov 30, 2022
a0320a4
refactor: change type and methods in MediaRange.ts
hinzzx Nov 30, 2022
1d3d349
refactor: correct layout size values
hinzzx Nov 30, 2022
26dc46f
fix: code review changes, test are now passing
hinzzx Dec 1, 2022
c2981de
fix: code review change
hinzzx Dec 2, 2022
3338b99
fix: code review change
hinzzx Dec 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 51 additions & 51 deletions packages/base/src/MediaRange.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,36 @@
const querySets = new Map<string, RangeSet>();
type Range = Map<string, Array<number>>;

type RangeSet = {
borders: Array<number>,
names: Array<string>,
const mediaRanges = new Map<string, Range>();

const DEAFULT_RANGE_SET: Range = new Map<string, Array<number>>();
DEAFULT_RANGE_SET.set("S", [0, 599]);
DEAFULT_RANGE_SET.set("M", [600, 1023]);
DEAFULT_RANGE_SET.set("L", [1024, 1439]);
DEAFULT_RANGE_SET.set("XL", [1440, Infinity]);

/**
* Enumeration containing the names and settings of predefined screen width media query range sets.
*
* @namespace
* @name MediaRange.RANGESETS
* @public
*/
enum RANGESETS {
/**
* A 4-step range set (S-M-L-XL).
*
* The ranges of this set are:
* <ul>
* <li><code>"S"</code>: For screens smaller than 600 pixels.</li>
* <li><code>"M"</code>: For screens greater than or equal to 600 pixels and smaller than 1024 pixels.</li>
* <li><code>"L"</code>: For screens greater than or equal to 1024 pixels and smaller than 1440 pixels.</li>
* <li><code>"XL"</code>: For screens greater than or equal to 1440 pixels.</li>
* </ul>
*
* @name MediaRange.RANGESETS.RANGE_4STEPS
* @public
*/
RANGE_4STEPS = "4Step",
}

/**
Expand All @@ -23,15 +51,11 @@ type RangeSet = {
*
* @param {string} name The name of the range set to be initialized.
* The name must be a valid id and consist only of letters and numeric digits.
* @param {int[]} [borders] The range borders
* @param {string[]} [names] The names of the ranges. The names must be a valid id and consist only of letters and digits.
* @param {Range} [range] The given range set.
* @name MediaRange.initRangeSet
*/
const _initRangeSet = (name: string, borders: Array<number>, names: Array<string>) => {
querySets.set(name, {
borders,
names,
});
const initRangeSet = (name: string, range: Range) => {
mediaRanges.set(name, range);
};

/**
Expand All @@ -41,56 +65,32 @@ const _initRangeSet = (name: string, borders: Array<number>, names: Array<string
* otherwise it is determined for the current window size.
*
* @param {string} name The name of the range set. The range set must be initialized beforehand ({@link MediaRange.initRangeSet})
* @param {int} [width] An optional width, based on which the range should be determined;
* If <code>width</code> is not provided, the window size will be used.
* @param {number} [width] An optional width, based on which the range should be determined;
* If <code>width</code> is not provided, the window size will be used.
* @returns {string} The name of the current active interval of the range set.
*
* @name MediaRange.getCurrentRange
* @function
* @public
*/
const _getCurrentRange = (name: string, width = window.innerWidth) => {
const querySet = querySets.get(name);
let i = 0;
const getCurrentRange = (name: string, width = window.innerWidth): string => {
let rangeSet = mediaRanges.get(name);

if (!querySet) {
return null;
if (!rangeSet) {
rangeSet = mediaRanges.get(RANGESETS.RANGE_4STEPS)!;
}

for (; i < querySet.borders.length; i++) {
if (width < querySet.borders[i]) {
return querySet.names[i];
let currentRangeName;

rangeSet.forEach((value, key) => {
if (Math.floor(width) >= value[0] && Math.floor(width) <= value[1]) {
Copy link
Member

Choose a reason for hiding this comment

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

Better introduce a new const for "Math.floor(width)" before the loop, because now we call "Math.floor" twice per iteration , while it can be calculated just once.
f.e.
const effectiveWidth = Math.floor(width)

Copy link
Contributor Author

@hinzzx hinzzx Dec 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi Ilhan, thanks for the tips and recommendations,

After some (manual) testing and debugging, I came to the following conclusion:

Following the logic that width: 1023px => "S" size and width: 1023.5px => M size, I found out that even without our change with the Math.floor(), the width value, our method getCurrentRange recieves is always rounded ( with Math.round logic, which returns the bigger integer after .5 and the smaller one below .5 ). With that said I even think that we might need to revert the Math.floor() change in order to keep the old logic, since our method never recieves a floating number.

Please correct me, if Im wrong.

Best regards,
Stoyan

currentRangeName = key;
}
}
});

return querySet.names[i];
return currentRangeName || [...rangeSet.keys()][0];
};

/**
* Enumeration containing the names and settings of predefined screen width media query range sets.
*
* @namespace
* @name MediaRange.RANGESETS
* @public
*/
enum RANGESETS {
/**
* A 4-step range set (S-M-L-XL).
*
* The ranges of this set are:
* <ul>
* <li><code>"S"</code>: For screens smaller than 600 pixels.</li>
* <li><code>"M"</code>: For screens greater than or equal to 600 pixels and smaller than 1024 pixels.</li>
* <li><code>"L"</code>: For screens greater than or equal to 1024 pixels and smaller than 1440 pixels.</li>
* <li><code>"XL"</code>: For screens greater than or equal to 1440 pixels.</li>
* </ul>
*
* @name MediaRange.RANGESETS.RANGE_4STEPS
* @public
*/
RANGE_4STEPS = "4Step",
}

/**
* API for screen width changes.
*
Expand All @@ -100,10 +100,10 @@ enum RANGESETS {

const MediaRange = {
RANGESETS,
initRangeSet: _initRangeSet,
getCurrentRange: _getCurrentRange,
initRangeSet,
getCurrentRange,
};

MediaRange.initRangeSet(MediaRange.RANGESETS.RANGE_4STEPS, [600, 1024, 1440], ["S", "M", "L", "XL"]);
MediaRange.initRangeSet(MediaRange.RANGESETS.RANGE_4STEPS, DEAFULT_RANGE_SET);

export default MediaRange;
8 changes: 5 additions & 3 deletions packages/base/src/util/FocusableElements.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,21 @@ import isElementHidden from "./isElementHidden.js";
import isElementClickable from "./isElementClickable.js";
import { instanceOfUI5Element } from "../UI5Element.js";

type FocusableElementPromise = Promise<HTMLElement | null>;

const isFocusTrap = (el: HTMLElement) => {
return el.hasAttribute("data-ui5-focus-trap");
};

const getFirstFocusableElement = async (container: HTMLElement, startFromContainer: boolean): Promise<HTMLElement | null> => {
const getFirstFocusableElement = async (container: HTMLElement, startFromContainer?: boolean): FocusableElementPromise => {
if (!container || isElementHidden(container)) {
return null;
}

return findFocusableElement(container, true, startFromContainer);
};

const getLastFocusableElement = async (container: HTMLElement, startFromContainer: boolean) => {
const getLastFocusableElement = async (container: HTMLElement, startFromContainer?: boolean): FocusableElementPromise => {
if (!container || isElementHidden(container)) {
return null;
}
Expand All @@ -26,7 +28,7 @@ const isElemFocusable = (el: HTMLElement) => {
return el.hasAttribute("data-ui5-focus-redirect") || !isElementHidden(el);
};

const findFocusableElement = async (container: HTMLElement, forward: boolean, startFromContainer?: boolean): Promise<HTMLElement | null> => {
const findFocusableElement = async (container: HTMLElement, forward: boolean, startFromContainer?: boolean): FocusableElementPromise => {
let child: HTMLElement | undefined;

if (container.shadowRoot) {
Expand Down