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

Manipulator: Add JSON parse support #35077

Merged
merged 12 commits into from
Apr 21, 2022
Merged

Manipulator: Add JSON parse support #35077

merged 12 commits into from
Apr 21, 2022

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Sep 28, 2021

A proposal trying to solve the JSON data-attributes parsing issue.

Merging this PR, components will be able to parse JSON string values from element data attributes as configuration values.
Edit: As an experimental add on, component elements can have a data-bs-config attribute that can house all the component config as JSON string. In case an element has data-bs-config='{"foo":123}' and data-bs-foo="456" attributes, final foo value will be 456, as the separate data attributes will override values given on data-bs-config

So the final config for each components will be a result of:

  • Defaults
  • data-bs-config
  • rest data attributes
  • configuration object given during instance initialization

I would appreciate any thoughts or doubts on it

Refs #34259, refs #34321, refs #35880
closes #34465

TODO:

  • Document it

@GeoSot GeoSot requested a review from a team September 29, 2021 10:01
@GeoSot GeoSot force-pushed the gs/parse-json-config branch 3 times, most recently from 1fc32ce to f92dea6 Compare October 5, 2021 23:16
@XhmikosR XhmikosR changed the title Manipulator: Add json parse support Manipulator: Add JSON parse support Mar 11, 2022
@XhmikosR XhmikosR added this to In progress in v5.3.0 via automation Mar 12, 2022
@XhmikosR XhmikosR added this to In progress in v5.2.0 via automation Mar 12, 2022
@GeoSot GeoSot marked this pull request as ready for review March 13, 2022 10:49
@GeoSot GeoSot force-pushed the gs/parse-json-config branch 3 times, most recently from 7eef37a to 839e385 Compare April 7, 2022 23:27
v5.2.0 automation moved this from In progress to Reviewer approved Apr 13, 2022
@@ -44,7 +52,7 @@ const Manipulator = {
}

const attributes = {}
const bsKeys = Object.keys(element.dataset).filter(key => key.startsWith('bs'))
const bsKeys = Object.keys(element.dataset).filter(key => key.startsWith('bs') && !key.startsWith('bsConfig'))

for (const key of bsKeys) {
let pureKey = key.replace(/^bs/, '')
Copy link
Member

Choose a reason for hiding this comment

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

@GeoSot shouldn't this remove bsConfig too?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I want to filter bsConfig out of given results . My purpose is to can be accessed only on purpose and not by massive get

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand. Right now, bsConfig is not normalized, right? Shouldn't it be normalized here?

Copy link
Member Author

@GeoSot GeoSot Apr 14, 2022

Choose a reason for hiding this comment

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

here (getDataAttributes) is omitted, on purpose (of mass getting).

We may need to think of normalization only in config.js usage, but I didn't do it, as the normalization of the json string should have be done already from the developer. We after all validate the final object.

(Check the tests please too. I think they cover your thoughts on line 80)

@arcanisgk
Copy link

Hi, I'm new to this...why did the test fail due to the disconnected test?

@arcanisgk
Copy link

I must remind you that in html the values of an attribute are enclosed in double quotes:
this semantically or in html code it is invalid:
data-bs-config='{"foo":123}'

the solution would be something like this:

  • php to generate the html (any server side mus implement this behavior):
    <div data-bs-config="<?php echo htmlspecialchars(json_encode($dataParams), ENT_QUOTES, 'UTF-8'); ?>">

  • javascript is the same:
    const bsKeys = Object.keys(element.dataset).filter(key => key.startsWith('bs') && !key.startsWith('bsConfig'))

@GeoSot
Copy link
Member Author

GeoSot commented Apr 20, 2022

Hi, I'm new to this...why did the test fail due to the disconnected test?

If anyone could enlighten me too, I would be much happier. 😕

GeoSot and others added 11 commits April 21, 2022 08:58
…he `data-bs-config` attribute.

Experimental ! ! !
The `bs-config` attribute will be reserved and omitted during `getDataAttributes` parsing

With this commit, every component, will create its config obj, using
 * Defaults
 * data-bs-config
 * rest data attributes
 * configuration object given during instance initialization
@mdo
Copy link
Member

mdo commented Apr 21, 2022

@GeoSot @XhmikosR Is this okay to merge?

@GeoSot
Copy link
Member Author

GeoSot commented Apr 21, 2022

It's ok by me. XhmikosR may have the last word

@XhmikosR XhmikosR merged commit 584600b into main Apr 21, 2022
@XhmikosR XhmikosR deleted the gs/parse-json-config branch April 21, 2022 18:41
v5.2.0 automation moved this from Reviewer approved to Done Apr 21, 2022
v5.3.0 automation moved this from In progress to Done Apr 21, 2022
@XhmikosR XhmikosR removed this from Done in v5.3.0 Apr 21, 2022
@Frenzie
Copy link
Contributor

Frenzie commented Aug 23, 2022

What if you want your string that looks like JSON to be treated like a string?

Edit:
PS For anybody else who's hit by this change, here's a quick workaround:

const content = popoverTriggerEl.getAttribute('data-bs-content');
if (content.startsWith('{')) {
  popoverTriggerEl.setAttribute('data-bs-content', content.replace('{', '&#123;'));
}

@Frenzie
Copy link
Contributor

Frenzie commented Aug 23, 2022

For example, taking from the manual https://getbootstrap.com/docs/5.2/components/popovers/#live-demo:

<button type="button" class="btn btn-lg btn-danger" data-bs-toggle="popover" data-bs-title="Popover title" data-bs-content='{"id":1234, "text":"This has worked for over a decade."}'>Click to toggle popover</button>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

Passing object as data attibute (popover, data-bs-delay)
5 participants