New feature: Add custom ad banner #1202

jasonzysun · 2023-09-21T08:18:09Z

In addition to the three methods, adbutler, coinzilla and slise, provided in the adBanner, a custom adBanner method has been added. The method of obtaining configuration files refers to NEXT_PUBLIC_MarkeTPLACE_CONFIG_URL and advertising display method refer to slise.
I did not use the two testing methods mentioned in the Contribution guide, but I have applied this configuration in Qitmeer's block browser and can access qng.qitmeer.io for viewing.

Let useCustomBanners() always obtains the latest data instead of downloading it at runtime

tom2drum

Can you also add the variable of the config URL to the list of downloaded assets, here - deploy/scripts/download_assets.sh? Then use helper getExternalAssetFilePath in the app config file to get the public path to the file in application code.

docs/ENVS.md

nextjs/nextjs-routes.d.ts

types/client/adCustomConfig.ts

ui/shared/ad/CustomAdBanner.tsx

ui/shared/ad/useCustomBanners.tsx

jasonzysun · 2023-09-22T03:36:45Z

OK, I'll add the variable of the config URL to the list of downloaded assets, and use helper getExternalAssetFilePath in the app config file to get the public path to the file in application code. I've done that at first commit but remove it at second because at that time I wanted this part of the advertisement to be automatically adjusted with modifications to external files, rather than fixed at startup. Now I think we can add a separate environment variable to control the use of this feature.

jasonzysun · 2023-09-22T03:38:40Z

I'll add another env variables like NEXT_PUBLIC_DISABLE_DOWNLOAD_AT_RUM_TIME to realize that in other pr.

2. Putting interval in the custom ad config and update the schema. 3. Add the variable of the config URL to the list of downloaded assets and use helper getExternalAssetFilePath in the app config file to get the public path to the file in application code. 4. Fix some variables' name and docs.

jasonzysun · 2023-09-22T06:08:37Z

@tom2drum Please check again.

tom2drum

Great work! Leave some comments about the code style and types. Feel free to fix them.

deploy/tools/envs-validator/schema.ts

docs/ENVS.md

types/client/ad.ts

ui/shared/ad/CustomAdBanner.tsx

tom2drum · 2023-09-22T13:32:40Z

I'll add another env variables like NEXT_PUBLIC_DISABLE_DOWNLOAD_AT_RUM_TIME to realize that in other pr.

I don't really think that we need this kind of setting. In general, the app should not break if the remote config is suddenly changed. Configuration should be persistent for the running instance. That's why we download all these files at run-time. If you need another configuration for your app, then re-run the container with new ENVs and you're good to go. It should not be any different than for other (non-json like) ENVs.

jasonzysun · 2023-09-22T13:44:59Z

I'll add another env variables like NEXT_PUBLIC_DISABLE_DOWNLOAD_AT_RUM_TIME to realize that in other pr.

I don't really think that we need this kind of setting. In general, the app should not break if the remote config is suddenly changed. Configuration should be persistent for the running instance. That's why we download all these files at run-time. If you need another configuration for your app, then re-run the container with new ENVs and you're good to go. It should not be any different than for other (non-json like) ENVs.

Why not leave this as an optional configuration for deployment users to decide?

jasonzysun · 2023-09-22T13:53:38Z

I'll add another env variables like NEXT_PUBLIC_DISABLE_DOWNLOAD_AT_RUM_TIME to realize that in other pr.

I don't really think that we need this kind of setting. In general, the app should not break if the remote config is suddenly changed. Configuration should be persistent for the running instance. That's why we download all these files at run-time. If you need another configuration for your app, then re-run the container with new ENVs and you're good to go. It should not be any different than for other (non-json like) ENVs.

Check this issue #1206 , that's anothe reason why I think this is necessary.

tom2drum · 2023-09-22T14:01:23Z

Why not leave this as an optional configuration for deployment users to decide?

It is fallible and potentially can break app integrity. I don't see any reason why we should make an exception for some config properties and allow them to be dynamic and not persistent. Such behavior should not be accepted on the general level of the app, imho. But users who really need this for some reason are always free to create a fork and make amends.

tom2drum · 2023-09-22T14:03:10Z

Check this issue #1206 , that's anothe reason why I think this is necessary.

I will take a look later when I have time. But I don't think it is going to change the approach.

jasonzysun · 2023-09-22T14:14:06Z

Why not leave this as an optional configuration for deployment users to decide?

It is fallible and potentially can break app integrity. I don't see any reason why we should make an exception for some config properties and allow them to be dynamic and not persistent. Such behavior should not be accepted on the general level of the app, imho. But users who really need this for some reason are always free to create a fork and make amends.

I fully understand your considerations. If you think this is not a feature that most users need, I will only implement it on my own branch.

ui/shared/ad/CustomAdBanner.tsx

… play order setting; 2. Update schema and doc; 3. Modify some variable names to improve code readability

jasonzysun · 2023-09-24T16:15:51Z

@tom2drum Please check the latest update. I've developed at https://testnet-qng.qitmeer.io/

# Conflicts: # deploy/scripts/download_assets.sh

tom2drum · 2023-09-26T15:16:29Z

docs/ENVS.md

+| banners | `array` | List of banners with their properties. Refer to the "Custom banners configuration properties" section below. | Required | - | See below |
+| interval | `number` | Duration (in milliseconds) for how long each banner will be displayed. | - | 60000 | `6000` |
+| randomStart | `boolean` | Set to true to randomly start playing advertisements from any position in the array | - | `false` | `true` |
+| randomNextAd | `boolean` | Set to ture to randomly play advertisements | - | `false` | `true` |


tom2drum · 2023-09-26T15:21:00Z

ui/shared/ad/CustomAdBanner.tsx

+  const configUrl = (feature.isEnabled && feature.provider === 'custom') ? feature.configUrl : '';
+
+  const apiFetch = useFetch();
+  const { data: adConfig } = useQuery<unknown, ResourceError<unknown>, AdCustomConfig>(


I think we need to manage the following states of the component as well:

loading - while the config is loading it is good to show a skeleton

error - if the request fails then render nothing (null)

Good idea, I will add these two states

ui/shared/ad/CustomAdBanner.tsx

2. Specify a skeleton as a fallback prop for the Image

2.use 'test(urlTest)' instead of 'url()'

jasonzysun · 2023-10-08T13:45:50Z

@tom2drum please check latest version

tom2drum · 2023-10-11T13:49:17Z

deploy/tools/envs-validator/schema.ts

+  .object()
+  .shape({
+    text: yup.string(),
+    url: yup.string().test(urlTest),


Isn't it required, is it?

tom2drum · 2023-10-11T13:51:45Z

docs/ENVS.md

 | NEXT_PUBLIC_AD_ADBUTLER_CONFIG_DESKTOP | `{ id: string; width: string; height: string }` | Placement config for desktop Adbutler banner | - | - | `{'id':'123456','width':'728','height':'90'}` |
 | NEXT_PUBLIC_AD_ADBUTLER_CONFIG_MOBILE | `{ id: string; width: number; height: number }` | Placement config for mobile Adbutler banner | - | - | `{'id':'654321','width':'300','height':'100'}` |
+| NEXT_PUBLIC_AD_CUSTOM_CONFIG_URL | `string` | URL of configuration file (.json format only) which contains settings and list of custom banners that will be shown in the home page and token detail page. See below list of available properties for particular banner | - | - | `https://example.com/ad_custom_config.json` |
+
+#### Configuration properties


Suggested change

#### Configuration properties

#### Custom ad configuration properties

just a little bit more specific

ui/shared/ad/CustomAdBanner.tsx

tom2drum · 2023-10-11T14:03:56Z

ui/shared/ad/CustomAdBanner.tsx

+  const baseBanners = adConfig?.banners || [];
+  const randomStart = adConfig?.randomStart || false;
+  const randomNextAd = adConfig?.randomNextAd || false;
+  const banners = randomNextAd ? shuffle(baseBanners) : baseBanners;


I would probably do a memoization here, otherwise whenever component re-renders, the new sequence of banners will be generated, so it kind of affects randomness a little bit

tom2drum · 2023-10-11T14:14:23Z

deploy/tools/envs-validator/schema.ts

+    mobileImageUrl: yup.string().test(urlTest).required(),
+  });
+
+const adCustomConfigSchema = yup


Have you checked the schema validation localy as it is described in docs?

I guess we are missing .json() here, so the yup can parse the string to the actual JSON.

Also in deploy/tools/envs-validator/index.ts you need to replace ENV value with the content of the json-file in order to make the validator work.

appEnvs.NEXT_PUBLIC_AD_CUSTOM_CONFIG_URL = await getExternalJsonContent( './public/assets/ad_custom_config.json', appEnvs.NEXT_PUBLIC_AD_CUSTOM_CONFIG_URL, ) || '[]';

Can you please temporarily add this config ENV to the .env.main preset file to demonstrate that everything works as expected? I will run the validator in our CI then. After that, you can comment it out. I know it is not the best workflow, but we are trying to improve it.

@jasonzysun I have just merged some changes in the validation script, now it should be easier to add tests for the new variables. I've described all the necessary steps in the contribution guide. Check it out and let me know if you have any questions.

That's great to hear. I haven't been able to address this section of the content lately, but I'll take care of it in the coming days.

fascinated · 2023-10-13T02:31:09Z

Do I understand correctly that this approach preloads a set of ads from a dynamic JSON manifest once on blockscout app server start? Is there any way to refresh the set of loaded assets in this approach?

jasonzysun · 2023-10-13T02:45:15Z

Do I understand correctly that this approach preloads a set of ads from a dynamic JSON manifest once on blockscout app server start? Is there any way to refresh the set of loaded assets in this approach?

Yes, your understanding is correct. Due to the design of #1184 , the external customized configuration files referenced by the front-end will be downloaded to the Docker during startup. Therefore, if the customized advertising content in the configuration files is modified externally after startup, the current version of the front-end UI will not respond to changes in a timely manner, and the browser needs to be manually restarted.

fascinated · 2023-10-13T03:12:26Z

@jasonzysun got it, thank you!

I know you've done a lot of work on this, so I must ask - is there a reason you didn't choose an approach like specifying an external iframe (or some external JS that would insert the iframe into the ad slot) url via an ENV value?

This general practice is very common with display advertising systems + does allow you to manage the ad content, frequency, etc completely without making further changes to the blockscout/docker/etc instance.

jasonzysun · 2023-10-13T03:45:14Z

@jasonzysun got it, thank you!

I know you've done a lot of work on this, so I must ask - is there a reason you didn't choose an approach like specifying an external iframe (or some external JS that would insert the iframe into the ad slot) url via an ENV value?

This general practice is very common with display advertising systems + does allow you to manage the ad content, frequency, etc completely without making further changes to the blockscout/docker/etc instance.

In fact, the purpose of this custom feature is not to introduce a new advertising system, but to provide a simple feature that allows users to implement their own advertising with just one configuration file when they have copy and images.

jasonzysun added 2 commits September 20, 2023 18:43

Add custom ad banner

2f0d608

Modify the usage of NEXT_PUBLIC_AD_CUSTOM_CONFIG_URL

2e9c7f4

Let useCustomBanners() always obtains the latest data instead of downloading it at runtime

jasonzysun marked this pull request as ready for review September 21, 2023 14:37

tom2drum self-requested a review September 21, 2023 19:19

tom2drum requested changes Sep 21, 2023

View reviewed changes

Merge remote-tracking branch 'blockscout/main' into add_custom_ad_banner

07cc436

jasonzysun added 3 commits September 22, 2023 14:24

fix some error

e0fac6c

fix some error

20b7b8f

fix some error

3ee3677

tom2drum approved these changes Sep 22, 2023

View reviewed changes

github-actions bot deployed to review September 22, 2023 16:35 View deployment

isstuev requested changes Sep 23, 2023

View reviewed changes

ui/shared/ad/CustomAdBanner.tsx Show resolved Hide resolved

ui/shared/ad/CustomAdBanner.tsx Outdated Show resolved Hide resolved

1. Add randomStart and randomNextAd into config to provide a optional…

2869118

… play order setting; 2. Update schema and doc; 3. Modify some variable names to improve code readability

jasonzysun added 2 commits September 25, 2023 14:35

Merge remote-tracking branch 'blockscout/main' into add_custom_ad_banner

1e285de

Merge remote-tracking branch 'origin/main' into add_custom_ad_banner

9613380

# Conflicts: # deploy/scripts/download_assets.sh

tom2drum reviewed Sep 26, 2023

View reviewed changes

jasonzysun added 3 commits September 27, 2023 11:00

1. Manage the loading and error states of the component.

5b41dc1

2. Specify a skeleton as a fallback prop for the Image

fix width of Skeleton

88b86d1

Merge remote-tracking branch 'blockscout/main' into add_custom_ad_banner

3ece9af

jasonzysun added 2 commits October 8, 2023 21:18

1.fix 'ture' to 'true'

449699d

2.use 'test(urlTest)' instead of 'url()'

Randomly playing by shuffling the array order

815b67d

jasonzysun added 2 commits October 8, 2023 23:32

remove 'process.env.'

4131076

fix doc

a0c59fa

tom2drum reviewed Oct 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New feature: Add custom ad banner #1202

New feature: Add custom ad banner #1202

jasonzysun commented Sep 21, 2023 •

edited

Loading

tom2drum left a comment

jasonzysun commented Sep 22, 2023

jasonzysun commented Sep 22, 2023 •

edited

Loading

jasonzysun commented Sep 22, 2023

tom2drum left a comment

tom2drum commented Sep 22, 2023

jasonzysun commented Sep 22, 2023

jasonzysun commented Sep 22, 2023

tom2drum commented Sep 22, 2023

tom2drum commented Sep 22, 2023

jasonzysun commented Sep 22, 2023

jasonzysun commented Sep 24, 2023 •

edited

Loading

tom2drum Sep 26, 2023

tom2drum Sep 26, 2023

jasonzysun Sep 27, 2023

jasonzysun commented Oct 8, 2023

tom2drum Oct 11, 2023

tom2drum Oct 11, 2023

tom2drum Oct 11, 2023

tom2drum Oct 11, 2023

tom2drum Oct 18, 2023

jasonzysun Oct 19, 2023

fascinated commented Oct 13, 2023

jasonzysun commented Oct 13, 2023

fascinated commented Oct 13, 2023

jasonzysun commented Oct 13, 2023

	\| randomNextAd \| `boolean` \| Set to ture to randomly play advertisements \| - \| `false` \| `true` \|
	\| randomNextAd \| `boolean` \| Set to true to randomly play advertisements \| - \| `false` \| `true` \|

	#### Configuration properties
	#### Custom ad configuration properties

New feature: Add custom ad banner #1202

Are you sure you want to change the base?

New feature: Add custom ad banner #1202

Conversation

jasonzysun commented Sep 21, 2023 • edited Loading

tom2drum left a comment

Choose a reason for hiding this comment

jasonzysun commented Sep 22, 2023

jasonzysun commented Sep 22, 2023 • edited Loading

jasonzysun commented Sep 22, 2023

tom2drum left a comment

Choose a reason for hiding this comment

tom2drum commented Sep 22, 2023

jasonzysun commented Sep 22, 2023

jasonzysun commented Sep 22, 2023

tom2drum commented Sep 22, 2023

tom2drum commented Sep 22, 2023

jasonzysun commented Sep 22, 2023

jasonzysun commented Sep 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonzysun commented Oct 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fascinated commented Oct 13, 2023

jasonzysun commented Oct 13, 2023

fascinated commented Oct 13, 2023

jasonzysun commented Oct 13, 2023

jasonzysun commented Sep 21, 2023 •

edited

Loading

jasonzysun commented Sep 22, 2023 •

edited

Loading

jasonzysun commented Sep 24, 2023 •

edited

Loading